Closed Bug 1491504 Opened 6 years ago Closed 6 years ago

Shortcut blob responses from XHR and fetch when the URL is a blob URL

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: emk, Assigned: twisniewski)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

Currently, XHR/fetch will open an input stream from the channel, read the data from the stream, create a blob storage, write the data into the storage, and get a new blob from the storage. It is ridicuously inefficient. We could reuse the source blob for the response.
Summary: Shortcut XHR and fetch when the URL is a blob URL → Shortcut blob responses from XHR and fetch when the URL is a blob URL
Is reading blob url to a blob a common use case?
I had to pass dropped files to some third-party JS libraries that take only urls.
Does fetch do the same thing?
Fetch also uses MutableBlobStorage for blob responses. I could not find anything specific to blob URLs (except for rejecting non-GET requests) in the fetch code.
baku probably has salient thoughts here.
Flags: needinfo?(amarchesini)
Priority: -- → P3
So does this show up badly in performance profiles? Just wondering about maintenance cost here - and the more we add special cases to the code, the most likely there will be inconsistent behavior.
Flags: needinfo?(VYV03354)
(In reply to Andrew Overholt [:overholt] from comment #5) > baku probably has salient thoughts here. We already have a similar optimization for local files to blobs. We can add this extra optimization checking the channel is BlobURLChannel and in case, we can directly extra the blobImpl.
Flags: needinfo?(amarchesini)
Thomas, you implemented the local file to blob optimization. Do you want to take this as well? The code is similar: the if channel is a BlobURLChannel, ask for the internal blobImpl or its inputStream. If you don't have time, let me know, and I can work on it. Thanks!
Flags: needinfo?(twisniewski)
I may not have time before this weekend, but I'll assign myself once I get the time. If it's pressing and you want to beat me to it, feel free :)
Flags: needinfo?(twisniewski)
(In reply to Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) from comment #6) > So does this show up badly in performance profiles? I didn't think I had to show such a very obvious performance hit... We already have a similar optimization for file://. So I compared the result with and without optimization using a file:// url to the Windows 10 ISO (4.31GiB). with opt: 0.028 sec without opt: 33.282 sec (1000x slower) Also, the latter will hog the disk space.
Flags: needinfo?(VYV03354)
Attached file Test file for the record (deleted) —
Download this file and put Win10_1803_Japanese_x64.iso in the same directory.
Assignee: nobody → VYV03354
Resetting accidental assignment.
Assignee: VYV03354 → nobody
shortcut blob responses from XHR and fetch when the URL is a blob URL
Assignee: nobody → twisniewski
Thanks for the review, baku! I did a trivial rebase and fresh try-run just in case, but don't see anything worrying: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15db261d95f601575bbb36168b5f0aecdeb065db Requesting check-in.
Keywords: checkin-needed
Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c5ac0cb25238 shortcut blob responses from XHR and fetch when the URL is a blob URL; r=baku
Keywords: checkin-needed
Wat. Looks like I'm missing a header that throws off that build configuration, but not the ones I tried earlier. :andrei_ciure_, is there a way for me to use "mach try" to send jobs to that server in the future? I'm running with this command right now: >mach try -b do -p linux64,android-api-16,win64,macosx64 -u all -t none --no-retry
Flags: needinfo?(twisniewski) → needinfo?(aciure)
I can't help you with that, redirected the ni to Aryx. You can try to add jobs from treeherder using the "Add new jobs feature" on the left side of the push panel
Flags: needinfo?(aciure) → needinfo?(aryx.bugmail)
Thanks Andrei. For the record, I just checked the treeherder web UI, but I didn't see any labels resembling "linux x64 opt build-linux64-base-toolchains/opt" in the list that I could trigger. I do see Linux/Linux64 variants for opt, debug, pgo, nightly, devedition, asan, asan-reporter, addon, quantumrender, ccov, stylo-seq, dmd, and "noopt debug". Also one simply labelled "Toolchains", but without a bb task. Maybe I don't have sufficient privileges to see that one?
> I didn't see any labels resembling "linux x64 opt > build-linux64-base-toolchains/opt" in the list that I could trigger. vs. > I see see [...] Linux64 variants for opt [...] Isn't that the one you need? https://dxr.mozilla.org/mozilla-central/source/taskcluster/ci/build/linux.yml#266,272-273 says it's the 'Bb' job on Linux x64 opt.
Flags: needinfo?(aryx.bugmail)
Ah, I see now: it's a tier2 job. But I'm unable to trigger it on my end. The panel doesn't pop up when I click on "Bb" to try to add a new task in the UI (and even expanding the +35 expander to get to the tier-2 jobs doesn't often work; usually it just logs a JS console error "TypeError: this is undefined; can't access its "setState" property"). Aryx, could you please trigger the job for me on this try-run? https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dbaa60099668be219e35c57b07a63092c0e795e
Flags: needinfo?(aryx.bugmail)
Nevermind, I managed to get the job through.
Flags: needinfo?(aryx.bugmail)
And with the #include that I missed, a try-run shows Bb is passing now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dbaa60099668be219e35c57b07a63092c0e795e Re-requesting checkin.
Keywords: checkin-needed
Pushed by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8248b734eb88 shortcut blob responses from XHR and fetch when the URL is a blob URL; r=baku
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1497071
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: