Closed Bug 1491504 Opened 2 years ago Closed 2 years ago
Shortcut blob responses from XHR and fetch when the URL is a blob URL
290 bytes, application/octet-stream
1.20 KB, patch
|Details | Diff | Splinter Review|
46 bytes, text/x-phabricator-request
|Details | Review|
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.
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.
(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.
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!
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 :)
(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.
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
A try-run seems fine so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b95f485eb6511dc433975f729d0d6496530f7239
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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/c5ac0cb25238 shortcut blob responses from XHR and fetch when the URL is a blob URL; r=baku
Backed out changeset c5ac0cb25238 (bug 1491504) for FetchConsumer.cpp build bustages push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=203766661&revision=c5ac0cb2523842abe2fddcf109f78957a09e9cf3 failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=b26a70a0fe8f22ee4a5118c7c563ab98a115e692&selectedJob=203770690&searchStr=linux%2Cx64%2Copt%2Cbuild-linux64-base-toolchains%2Fopt%2C%28bb%29 backout: https://hg.mozilla.org/integration/autoland/rev/e936e01a2949eea1c8d06dc3485b09e531fd151a
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.
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
Nevermind, I managed to get the job through.
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.
Pushed by email@example.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
You need to log in before you can comment on or make changes to this bug.