Avoid copying script source buffers where possible

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: jonco, Assigned: jonco)

Tracking

61 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(9 attachments, 1 obsolete attachment)

Some of our compilation APIs take a SourceBufferHolder which can be used to let the JS engine take ownership of a buffer containing script source and avoid copying it.  Our asynchronous compilation APIs do not permit this however, and for the synchronous ones this option is not always used.

This bug aims to extend the use of SourceBufferHolder to asynchronous compilation APIs and to use this feature in the browser where possible.
This patch removes synchronous compilation APIs that take char16_t pointers and makes everything use SourceBufferHolder instead.

I considered making the ownership parameter of the SourceBufferHolder default to NoOwnership but I left it as is to keep this explicit.
Attachment #8991834 - Flags: review?(jdemooij)
Make async compilation APIs take SourceBufferHolder too.  Use one of these to keep the data in the parse task.
Attachment #8991835 - Flags: review?(jdemooij)
Add an SourceBufferHolder constructor that takes a UniquePtr to the buffer and transfers ownership.
Attachment #8991836 - Flags: review?(jdemooij)
Refactor GetScriptSource to not take an inlineData parameter.  Previously this was used to hold the script source chars for inline elements.  Instead, copy the buffer and transfer ownership to the SourceBufferHolder returned.

This looks like an extra copy but it's not - the JS engine would have copied the data itself if ownership was not transferred.

It would be great if we could get the script text straight into a JS heap allocated buffer and avoid this copy too.  I don't know if there's any way to do that?
Attachment #8991838 - Flags: review?(amarchesini)
This patch refactors ScriptLoader::AttemptAsyncScriptCompile() to return a boolean (via an out parameter) indicating whether async compilation was possible rather than returning NS_ERROR_FAILURE if it was not.  Thus all error codes indicate actual errors rather than things like 'script was longer than the off-thread compile threshold'.

Then we handle all errors from this method, rather than ignoring and compiling on the main thread.

The reason is that we can't re-try compilation if we've already transferred ownership of the source buffer to the JS engine.
Attachment #8991844 - Flags: review?(amarchesini)
Add JSMallocAllocPolicy as a replacement for MallocAllocPolicy in the browser that allocates data on the JS heap, and doesn't account memory use.  The reason for the latter is that this is primarily for source text buffers which won't be associated with any GC thing and using them to trigger GC won't have any effect.

(After I wrote that I wondered about how script source data is accounted which is where this buffer ends up I believe.  Maybe this data should be accounted after all?)
Attachment #8991846 - Flags: review?(jdemooij)
Finally, make the script loader use JSMallocAllocPolicy for the Vector used to hold the script source and transfer that buffer to the JS engine.
Attachment #8991865 - Flags: review?(amarchesini)
There are a few places where we still copy source buffers, mostly when we're compiling data in a string e.g. an nsAString.  There doesn't seem to be a way to steal the buffer from those.
Comment on attachment 8991834 [details] [diff] [review]
source-buffer-1-sync-apis

Review of attachment 8991834 [details] [diff] [review]:
-----------------------------------------------------------------

I agree it's nice to be explicit about (not) giving up ownership.
Attachment #8991834 - Flags: review?(jdemooij) → review+
Comment on attachment 8991835 [details] [diff] [review]
source-buffer-2-async-apis

Review of attachment 8991835 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xul/XULDocument.cpp
@@ -2380,5 @@
>                  // alive until the compilation finishes.
>                  mOffThreadCompiling = true;
> -                // If the JS engine did not take the source buffer, then take
> -                // it back here to ensure it remains alive.
> -                mOffThreadCompileStringBuf = srcBuf.take();

Can we MOZ_RELEASE_ASSERT(!srcBuf.ownsChars()); with a comment saying the JS engine took ownership and will ensure the buffer stays alive?
Attachment #8991835 - Flags: review?(jdemooij) → review+
Comment on attachment 8991836 [details] [diff] [review]
source-buffer-3-unique-ptr

Review of attachment 8991836 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but maybe also request review from someone familiar with AsyncScriptCompiler, to make sure it's okay to steal mScriptText.
Attachment #8991836 - Flags: review?(jdemooij) → review+
Comment on attachment 8991838 [details] [diff] [review]
source-buffer-4-get-script-source

Review of attachment 8991838 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/script/ScriptLoader.cpp
@@ +1893,4 @@
>  
>    // If there's no script text, we try to get it from the element
>    if (aRequest->mIsInline) {
>      // XXX This is inefficient - GetText makes multiple

Is this comment still valid? GetText -> GetScriptText
Attachment #8991838 - Flags: review?(amarchesini) → review+
Attachment #8991844 - Flags: review?(amarchesini) → review+
Comment on attachment 8991865 [details] [diff] [review]
source-buffer-7-script-loader

Review of attachment 8991865 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/script/ScriptLoadRequest.h
@@ +173,5 @@
>  
> +  using ScriptTextBuffer = Vector<char16_t, 0, JSMallocAllocPolicy>;
> +  using BinASTSourceBuffer = Vector<uint8_t>;
> +
> +  const ScriptTextBuffer& ScriptText() const {

{ in a new line.

@@ +180,2 @@
>    }
> +  ScriptTextBuffer& ScriptText() {

same here and elsewhere.
Attachment #8991865 - Flags: review?(amarchesini) → review+
Comment on attachment 8991836 [details] [diff] [review]
source-buffer-3-unique-ptr

Requesting additional review for AsyncScriptCompiler changes, specifically if it's OK to transfer ownership of mScriptText buffer to the JS engine when compiling.
Attachment #8991836 - Flags: review?(kmaglione+bmo)
(In reply to Andrea Marchesini [:baku] from comment #12)
> Is this comment still valid? GetText -> GetScriptText

Looks like the change to GetScriptText was made in bug 235826... probably this is not valid any more.  I'll take this out.
(In reply to Jon Coppeard (:jonco) from comment #6)
> (After I wrote that I wondered about how script source data is accounted
> which is where this buffer ends up I believe.  Maybe this data should be
> accounted after all?)

There's also the source compression thing to consider: it replaces the uncompressed source with a compressed buffer. The compressed data is put in the process-wide SharedImmutableStringsCache. However, compression tasks are enqueued and actually start running at at the next GC..

Given the complexity here and the fact that GC can be triggered between allocation and creating the ScriptSource object (when parsing off-thread, I think?) not accounting probably makes sense. But I don't really know.
Comment on attachment 8991846 [details] [diff] [review]
source-buffer-6-js-alloc-policy

Review of attachment 8991846 [details] [diff] [review]:
-----------------------------------------------------------------

Isn't this what js::SystemAllocPolicy is for?
(In reply to Jan de Mooij [:jandem] from comment #17)
> Isn't this what js::SystemAllocPolicy is for?

There's one difference - this doesn't participate in simulated OOM testing.  We probably want to have these as separate things anyway since this is a public API.

I updated the patch to factor out common alloc policy methods into a base class.
Attachment #8991846 - Attachment is obsolete: true
Attachment #8991846 - Flags: review?(jdemooij)
Attachment #8992347 - Flags: review?(jdemooij)
Attachment #8991836 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8992347 [details] [diff] [review]
source-buffer-6-js-alloc-policy v2

Review of attachment 8992347 [details] [diff] [review]:
-----------------------------------------------------------------

SystemAllocPolicy is also used outside SM: https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/dom/base/CustomElementRegistry.h#490

This makes sense though, especially with the base class.

::: js/src/jsapi.h
@@ +1319,5 @@
> +/*
> + * A replacement for MallocAllocPolicy that allocates in the JS heap and adds no
> + * extra behaviours.
> + *
> + * This currently used for allocating source buffers for parsing. Since these

Nit: s/This/This is/
Attachment #8992347 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #19)
> SystemAllocPolicy is also used outside SM:

I was not aware of that!

Cheers for the reviews.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dde64fbe2f0d
Make synchronous compile APIs take SourceBufferHolders exclusively r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/74d10b32b3ea
Make asynchronous compile APIs take SourceBufferHolders r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3829c85f650
Allow construction of a SourceBufferHolder from a UniquePtr r=jandem r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/104817d51d1b
Refactor ScriptLoader::GetScriptSource() to remove inline data argument r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf96bd78dc11
Don't ignore errors returned from ScriptLoader::AttemptAsyncScriptCompile() r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/623af73419eb
Add JSMallocAllocPolicy to let gecko allocate data structures using the JS heap r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/e91802969fb7
Allocate script loader source buffers on the JS heap and pass ownership when compiling r=baku
This broke the rust bindings like so:

[task 2018-07-17T13:35:20.492Z]    Compiling js v0.1.4 (file:///builds/worker/workspace/build/src/js/rust)
[task 2018-07-17T13:35:20.492Z]      Running `rustc --crate-name build_script_build js/rust/build.rs --crate-type bin --emit=dep-info,link -C opt-level=1 -C debuginfo=2 -C debug-assertions=on --cfg 'feature="debugmozjs"' --cfg 'feature="mozjs_sys"' -C metadata=7e70562eba5fd21a -C extra-filename=-7e70562eba5fd21a --out-dir /builds/worker/workspace/build/src/target/debug/build/js-7e70562eba5fd21a -C incremental=/builds/worker/workspace/build/src/target/debug/incremental -L dependency=/builds/worker/workspace/build/src/target/debug/deps --extern bindgen=/builds/worker/workspace/build/src/target/debug/deps/libbindgen-1cad4bbeb212e01b.rlib --extern cmake=/builds/worker/workspace/build/src/target/debug/deps/libcmake-6d50e6c1888da96f.rlib --extern env_logger=/builds/worker/workspace/build/src/target/debug/deps/libenv_logger-3f353f775d164cc8.rlib --extern glob=/builds/worker/workspace/build/src/target/debug/deps/libglob-3c28eb7ff2197a92.rlib -L native=/builds/worker/workspace/build/src/target/debug/build/libloading-d397eae4f2ab8722/out`
[task 2018-07-17T13:37:37.530Z]      Running `rustc --crate-name mozjs_sys js/src/lib.rs --crate-type lib --emit=dep-info,link -C opt-level=1 -C debuginfo=2 -C debug-assertions=on --cfg 'feature="debugmozjs"' -C metadata=9b52510cd2a6637a -C extra-filename=-9b52510cd2a6637a --out-dir /builds/worker/workspace/build/src/target/debug/deps -C incremental=/builds/worker/workspace/build/src/target/debug/incremental -L dependency=/builds/worker/workspace/build/src/target/debug/deps --extern libc=/builds/worker/workspace/build/src/target/debug/deps/liblibc-c71ecf5670ff3bca.rlib --extern libz_sys=/builds/worker/workspace/build/src/target/debug/deps/liblibz_sys-bf82ad5826b39b72.rlib -L native=/builds/worker/workspace/build/src/target/debug/build/mozjs_sys-b1f5f1a57e68fd8e/out/js/src/build -L native=/builds/worker/workspace/build/src/target/debug/build/mozjs_sys-b1f5f1a57e68fd8e/out/js/src -L native=/builds/worker/workspace/build/src/target/debug/build/mozjs_sys-b1f5f1a57e68fd8e/out/dist/bin -l static=js_static -l nspr4 -l stdc++ -L native=/usr/lib/x86_64-linux-gnu`
[task 2018-07-17T13:37:37.531Z]      Running `/builds/worker/workspace/build/src/target/debug/build/js-7e70562eba5fd21a/build-script-build`
[task 2018-07-17T13:37:37.533Z]      Running `/builds/worker/workspace/build/src/target/debug/build/js-7e70562eba5fd21a/build-script-build`
[task 2018-07-17T13:37:47.935Z]      Running `rustc --crate-name js js/rust/src/lib.rs --emit=dep-info,link -C debuginfo=2 --test --cfg 'feature="debugmozjs"' --cfg 'feature="mozjs_sys"' -C metadata=3271c2a00ecb3ff6 -C extra-filename=-3271c2a00ecb3ff6 --out-dir /builds/worker/workspace/build/src/target/debug/deps -C incremental=/builds/worker/workspace/build/src/target/debug/incremental -L dependency=/builds/worker/workspace/build/src/target/debug/deps --extern lazy_static=/builds/worker/workspace/build/src/target/debug/deps/liblazy_static-5e270a024aff19c7.rlib --extern libc=/builds/worker/workspace/build/src/target/debug/deps/liblibc-c71ecf5670ff3bca.rlib --extern log=/builds/worker/workspace/build/src/target/debug/deps/liblog-6fa8c6c3a4686807.rlib --extern mozjs_sys=/builds/worker/workspace/build/src/target/debug/deps/libmozjs_sys-9b52510cd2a6637a.rlib --extern num_traits=/builds/worker/workspace/build/src/target/debug/deps/libnum_traits-66c0a1d61fa3acb6.rlib -L native=/builds/worker/workspace/build/src/target/debug/build/js-a81784c0626bfc1a/out/lib -l static=jsglue -L native=/builds/worker/workspace/build/src/target/debug/build/mozjs_sys-b1f5f1a57e68fd8e/out/js/src/build -L native=/builds/worker/workspace/build/src/target/debug/build/mozjs_sys-b1f5f1a57e68fd8e/out/js/src -L native=/builds/worker/workspace/build/src/target/debug/build/mozjs_sys-b1f5f1a57e68fd8e/out/dist/bin -L native=/usr/lib/x86_64-linux-gnu`
[task 2018-07-17T13:37:47.941Z]      Running `rustc --crate-name js js/rust/src/lib.rs --crate-type lib --emit=dep-info,link -C opt-level=1 -C debuginfo=2 -C debug-assertions=on --cfg 'feature="debugmozjs"' --cfg 'feature="mozjs_sys"' -C metadata=2e53ec46a132cda0 -C extra-filename=-2e53ec46a132cda0 --out-dir /builds/worker/workspace/build/src/target/debug/deps -C incremental=/builds/worker/workspace/build/src/target/debug/incremental -L dependency=/builds/worker/workspace/build/src/target/debug/deps --extern lazy_static=/builds/worker/workspace/build/src/target/debug/deps/liblazy_static-5e270a024aff19c7.rlib --extern libc=/builds/worker/workspace/build/src/target/debug/deps/liblibc-c71ecf5670ff3bca.rlib --extern log=/builds/worker/workspace/build/src/target/debug/deps/liblog-6fa8c6c3a4686807.rlib --extern mozjs_sys=/builds/worker/workspace/build/src/target/debug/deps/libmozjs_sys-9b52510cd2a6637a.rlib --extern num_traits=/builds/worker/workspace/build/src/target/debug/deps/libnum_traits-66c0a1d61fa3acb6.rlib -L native=/builds/worker/workspace/build/src/target/debug/build/js-3ca136f0899fec10/out/lib -l static=jsglue -L native=/builds/worker/workspace/build/src/target/debug/build/mozjs_sys-b1f5f1a57e68fd8e/out/js/src/build -L native=/builds/worker/workspace/build/src/target/debug/build/mozjs_sys-b1f5f1a57e68fd8e/out/js/src -L native=/builds/worker/workspace/build/src/target/debug/build/mozjs_sys-b1f5f1a57e68fd8e/out/dist/bin -L native=/usr/lib/x86_64-linux-gnu`
[task 2018-07-17T13:37:48.939Z] error[E0308]: mismatched types
[task 2018-07-17T13:37:48.939Z]    --> js/rust/src/rust.rs:237:55
[task 2018-07-17T13:37:48.939Z]     |
[task 2018-07-17T13:37:48.939Z] 237 |             if !JS::Evaluate2(self.cx(), options.ptr, ptr as *const u16, len as _, rval) {
[task 2018-07-17T13:37:48.939Z]     |                                                       ^^^^^^^^^^^^^^^^^ expected i8, found u16
[task 2018-07-17T13:37:48.939Z]     |
[task 2018-07-17T13:37:48.939Z]     = note: expected type `*const i8`
[task 2018-07-17T13:37:48.939Z]                found type `*const u16`
[task 2018-07-17T13:37:48.939Z] 
[task 2018-07-17T13:37:49.544Z] error: aborting due to previous error
[task 2018-07-17T13:37:49.544Z] 
[task 2018-07-17T13:37:49.544Z] For more information about this error, try `rustc --explain E0308`.
[task 2018-07-17T13:37:49.547Z] error[E0308]: mismatched types
[task 2018-07-17T13:37:49.547Z]    --> js/rust/src/rust.rs:237:55
[task 2018-07-17T13:37:49.547Z]     |
[task 2018-07-17T13:37:49.547Z] 237 |             if !JS::Evaluate2(self.cx(), options.ptr, ptr as *const u16, len as _, rval) {
[task 2018-07-17T13:37:49.547Z]     |                                                       ^^^^^^^^^^^^^^^^^ expected i8, found u16
[task 2018-07-17T13:37:49.547Z]     |
[task 2018-07-17T13:37:49.547Z]     = note: expected type `*const i8`
[task 2018-07-17T13:37:49.547Z]                found type `*const u16`
[task 2018-07-17T13:37:49.547Z] 
[task 2018-07-17T13:37:49.548Z] error: Could not compile `js`.
I think the problem is that I removed one of the overloads of JS::Evaluate in the first patch.  But I tried adding just that back in but still couldn't make the rust bindings compile locally.

Nick, do you know what's going on here?  Full error is in comment 22.

Ideally I want to make evaluate_script use SourceBufferHolder but I couldn't figure out how to do that either:

https://searchfox.org/mozilla-central/source/js/rust/src/rust.rs#237
Flags: needinfo?(jcoppeard) → needinfo?(nfitzgerald)
When bindgen encounters overloads, it starts adding a counter to the end to disambiguate. Assuming you didn't remove the overload that evaluate_script is actually using, then we probably just need to change that number on the end.

If you take a look at `js/rust/src/jsapi.rs`, you'll see that it includes the bindings that are mechanically generated in `js/rust/build.rs` and written into the "out" directory.

https://searchfox.org/mozilla-central/source/js/rust/src/jsapi.rs
https://searchfox.org/mozilla-central/source/js/rust/build.rs

Depending on build configuration files will be somewhere like `js/rust/target/**/build/out/jsapi[_debug].rs`. You should be able to grep through those files to find the correct overload.

I attempted to do this locally, but check_spidermonkey_style.py is throwing an exception on my current machine and the build isn't completing :-/

Let me know if anything doesn't make sense, or if you run into any problems. Happy to help!
Flags: needinfo?(nfitzgerald)
Summary: Avoid coping script source buffers where possible → Avoid copying script source buffers where possible
> Assuming you didn't remove the overload that evaluate_script is actually using

I removed the overload of Evaluate that takes a pointer and length to a source chars buffer to make callers use SourceBufferHolder instead.

Here's patch changes the rust code to use SourceBufferHolder.  This compiles but I don't really know what I'm doing here.  Does this look OK?
Attachment #8995468 - Flags: review?(nfitzgerald)
Comment on attachment 8995468 [details] [diff] [review]
fix-rust-bindings

Review of attachment 8995468 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Thanks for following through on this jonco :)
Attachment #8995468 - Flags: review?(nfitzgerald) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34fb24d52f08
Make synchronous compile APIs take SourceBufferHolders exclusively r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c83633262db
Make asynchronous compile APIs take SourceBufferHolders r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/6104ea971587
Allow construction of a SourceBufferHolder from a UniquePtr r=jandem r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc8f24dc3fc
Refactor ScriptLoader::GetScriptSource() to remove inline data argument r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/b82c2cf4b3f1
Don't ignore errors returned from ScriptLoader::AttemptAsyncScriptCompile() r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/02b27f8441be
Add JSMallocAllocPolicy to let gecko allocate data structures using the JS heap r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/45d3ffe3308e
Allocate script loader source buffers on the JS heap and pass ownership when compiling r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d18ea619ec
Fix rust bindings following JS::Evaluate signature change r=fitzgen
The problem here is that the script text length is used after compilation has started.  With the previous patches this would appear to be zero since the buffer had been passed to the JS engine.

The patch records the length separately.  I also refactored the script load handler code that appends data to the buffer so that it doesn't write into the allocated but uninitialised data at the end of the vector but just resizes the vector normally.
Flags: needinfo?(jcoppeard)
Attachment #8996366 - Flags: review?(amarchesini)
Attachment #8996366 - Flags: review?(amarchesini) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4204e7eaa385
Make synchronous compile APIs take SourceBufferHolders exclusively r=jandem r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b280769277af
Make asynchronous compile APIs take SourceBufferHolders r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/f43d114999ad
Allow construction of a SourceBufferHolder from a UniquePtr r=jandem r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce27ea285957
Add JSMallocAllocPolicy to let gecko allocate data structures using the JS heap r=jandem
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a05b0af027
Refactor ScriptLoader::GetScriptSource() to remove inline data argument r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c94a41970e2
Don't ignore errors returned from ScriptLoader::AttemptAsyncScriptCompile() r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/771c40992e31
Allocate script loader source buffers on the JS heap and pass ownership when compiling r=baku
Depends on: 1480966
You need to log in before you can comment on or make changes to this bug.