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

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
9 months ago
3 months ago

People

(Reporter: emk, Assigned: twisniewski)

Tracking

(Depends on 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

9 months ago
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

9 months 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
Is reading blob url to a blob a common use case?
Reporter

Comment 2

9 months ago
I had to pass dropped files to some third-party JS libraries that take only urls.
Assignee

Comment 3

9 months ago
Does fetch do the same thing?
Reporter

Comment 4

9 months 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.
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)
Assignee

Comment 9

9 months 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

9 months 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

9 months ago
Download this file and put Win10_1803_Japanese_x64.iso in the same directory.
Reporter

Comment 13

9 months ago
Resetting accidental assignment.
Assignee: VYV03354 → nobody
shortcut blob responses from XHR and fetch when the URL is a blob URL
Assignee

Updated

9 months ago
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

Comment 17

8 months 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
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

Comment 26

8 months 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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8248b734eb88
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Reporter

Updated

8 months ago
Depends on: 1497071
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.