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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: emk, Assigned: twisniewski)
References
(Depends on 1 open bug)
Details
Attachments
(3 files)
(deleted),
application/octet-stream
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-phabricator-request
|
Details |
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.
Reporter | ||
Updated•6 years ago
|
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
Comment 1•6 years ago
|
||
Is reading blob url to a blob a common use case?
Reporter | ||
Comment 2•6 years ago
|
||
I had to pass dropped files to some third-party JS libraries that take only urls.
Assignee | ||
Comment 3•6 years ago
|
||
Does fetch do the same thing?
Reporter | ||
Comment 4•6 years ago
|
||
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.
Comment 5•6 years ago
|
||
baku probably has salient thoughts here.
Flags: needinfo?(amarchesini)
Priority: -- → P3
Comment 6•6 years ago
|
||
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)
Comment 7•6 years ago
|
||
(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)
Comment 8•6 years ago
|
||
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)
Assignee | ||
Comment 9•6 years ago
|
||
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)
Reporter | ||
Comment 10•6 years ago
|
||
(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)
Reporter | ||
Comment 11•6 years ago
|
||
Download this file and put Win10_1803_Japanese_x64.iso in the same directory.
Reporter | ||
Comment 12•6 years ago
|
||
Assignee: nobody → VYV03354
Assignee | ||
Comment 14•6 years ago
|
||
shortcut blob responses from XHR and fetch when the URL is a blob URL
Assignee | ||
Comment 15•6 years ago
|
||
A try-run seems fine so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b95f485eb6511dc433975f729d0d6496530f7239
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → twisniewski
Assignee | ||
Comment 16•6 years ago
|
||
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
Comment 17•6 years ago
|
||
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
Comment 18•6 years ago
|
||
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
Flags: needinfo?(twisniewski)
Assignee | ||
Comment 19•6 years ago
|
||
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)
Comment 20•6 years ago
|
||
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)
Assignee | ||
Comment 21•6 years ago
|
||
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?
Comment 22•6 years ago
|
||
> 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)
Assignee | ||
Comment 23•6 years ago
|
||
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)
Assignee | ||
Comment 24•6 years ago
|
||
Nevermind, I managed to get the job through.
Flags: needinfo?(aryx.bugmail)
Assignee | ||
Comment 25•6 years ago
|
||
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
Comment 26•6 years ago
|
||
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
Comment 27•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•6 years ago
|
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•