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

Status: RESOLVED FIXED

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

Description

[Masatoshi Kimura [:emk]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Is reading blob url to a blob a common use case?

Comment 2

[Masatoshi Kimura [:emk]]

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

Comment 3

[Thomas Wisniewski [:twisniewski]]

Does fetch do the same thing?

Comment 4

[Masatoshi Kimura [:emk]]

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

[Andrew Overholt [:overholt]]

baku probably has salient thoughts here.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 7

[Andrea Marchesini [:baku]]

(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.

Comment 8

[Andrea Marchesini [:baku]]

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!

Comment 9

[Thomas Wisniewski [: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 :)

Comment 10

[Masatoshi Kimura [:emk]]

(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.

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Test file for the record

Download this file and put Win10_1803_Japanese_x64.iso in the same directory.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: [DO NOT LAND] patch to disable optimization for file:// for the record

Comment 13

[Masatoshi Kimura [:emk]]

Resetting accidental assignment.

Comment 14

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1491504 - shortcut blob responses from XHR and fetch when the URL is a blob URL; r?baku

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

Comment 15

[Thomas Wisniewski [:twisniewski]]

A try-run seems fine so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b95f485eb6511dc433975f729d0d6496530f7239

Comment 16

[Thomas Wisniewski [: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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5ac0cb25238
shortcut blob responses from XHR and fetch when the URL is a blob URL; r=baku

Comment 18

[Andrei Ciure[:aciure]]

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

Comment 19

[Thomas Wisniewski [:twisniewski]]

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

Comment 20

[Andrei Ciure[: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

Comment 21

[Thomas Wisniewski [:twisniewski]]

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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

> 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.

Comment 23

[Thomas Wisniewski [:twisniewski]]

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

Comment 24

[Thomas Wisniewski [:twisniewski]]

Nevermind, I managed to get the job through.

Comment 25

[Thomas Wisniewski [:twisniewski]]

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.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8248b734eb88
shortcut blob responses from XHR and fetch when the URL is a blob URL; r=baku

Comment 27

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8248b734eb88