Closed Bug 836515 Opened 11 years ago Closed 11 years ago

Don't wait for source compression before js execution.

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21
Project Flags:
blocking-b2g tef+
Tracking Flags:
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: jrmuizel, Assigned: Benjamin)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [FFOS_perf])

Attachments

(4 files, 1 obsolete file)

backport to b2g18
4.03 KB, patch
Details | Diff | Splinter Review
turn off Function.toString
666 bytes, patch
Details | Diff | Splinter Review
add APIs to control source saving
2.31 KB, patch
Details | Diff | Splinter Review
Allow source compression to run while executing the script
5.84 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review 
In the gaia email app we spend about 1.1 seconds waiting for source compression to finish during startup. This seems unnecessary as we shouldn't ever have to wait for compression to finish, as we have the original source until after the compression is done.

Here's a stack of us waiting:
#0  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:183
#1  0x40101254 in _normal_lock (mutex=0x41a8bba0) at bionic/libc/bionic/pthread.c:951
#2  pthread_mutex_lock (mutex=0x41a8bba0) at bionic/libc/bionic/pthread.c:1041
#3  0x415742c8 in PR_Lock (lock=0xfffffff5) at /home/jrmuizel/src/B2G/gecko/nsprpub/pr/src/pthreads/ptsynch.c:174
#4  0x40e78a5a in js::SourceCompressorThread::waitOnCompression (this=<value optimized out>) at /home/jrmuizel/src/B2G/gecko/js/src/jsscript.cpp:1004
#5  js::SourceCompressionToken::ensureReady (this=<value optimized out>) at /home/jrmuizel/src/B2G/gecko/js/src/jsscript.cpp:1184
#6  0x40ed88c0 in ~SourceCompressionToken (cx=0x0, scopeChain=..., callerFrame=0x43644e98, options=..., chars=0x44700008, length=1276722, source_=0x0, staticLevel=0) at /home/jrmuizel/src/B2G/gecko/js/src/jsscript.h:1154
#7  js::frontend::CompileScript (cx=0x0, scopeChain=..., callerFrame=0x43644e98, options=..., chars=0x44700008, length=1276722, source_=0x0, staticLevel=0) at /home/jrmuizel/src/B2G/gecko/js/src/frontend/BytecodeCompiler.cpp:249
#8  0x40dc6ea4 in JS::Evaluate (cx=0x42a5fbf0, obj=..., options=..., chars=0x0, length=1077758579, rval=0x0) at /home/jrmuizel/src/B2G/gecko/js/src/jsapi.cpp:5702
Blocks: Email-Startup
blocking-b2g: --- → tef?
I don't have the means to test this. Nobody could accuse this patch of being pretty, so let's see if it helps. Even if doesn't help this bug, it might do something for bug 821040.
Attachment #708377 - Flags: review?(jorendorff)
Whiteboard: [FFOS_perf]
One thing a realized when thinking about this, is that overlapping script compression with execution may not be sufficient to not wait. For example, the script may just install a few event handlers. We need to actually return to the event loop without waiting for script compression.
I realize; that's a project of much wider scope I'm afraid.
(In reply to :Benjamin Peterson from comment #3)
> I realize; that's a project of much wider scope I'm afraid.

Why is this?  I would imagine (not knowing this code) that you could just leave the source compressor thread running, and only block on it from the main thread if (a) the main thread actually needs the source for something, or (b) GC etc. that might erase references the compressor thread holds.  (This is similar to what we do for off thread compilation).  Does the compressor thread not hold a strong reference on the source it is compressing?
(In reply to Brian Hackett (:bhackett) from comment #4)
> (In reply to :Benjamin Peterson from comment #3)
> > I realize; that's a project of much wider scope I'm afraid.
> 
> Why is this?  I would imagine (not knowing this code) that you could just
> leave the source compressor thread running, and only block on it from the
> main thread if (a) the main thread actually needs the source for something,
> or (b) GC etc. that might erase references the compressor thread holds. 
> (This is similar to what we do for off thread compilation).  Does the
> compressor thread not hold a strong reference on the source it is
> compressing?

Exactly. The source is borrowed from the caller.
(In reply to :Benjamin Peterson from comment #5)
> (In reply to Brian Hackett (:bhackett) from comment #4)
> > (In reply to :Benjamin Peterson from comment #3)
> > > I realize; that's a project of much wider scope I'm afraid.
> > 
> > Why is this?  I would imagine (not knowing this code) that you could just
> > leave the source compressor thread running, and only block on it from the
> > main thread if (a) the main thread actually needs the source for something,
> > or (b) GC etc. that might erase references the compressor thread holds. 
> > (This is similar to what we do for off thread compilation).  Does the
> > compressor thread not hold a strong reference on the source it is
> > compressing?
> 
> Exactly. The source is borrowed from the caller.

Couldn't we create a reference counted wrapper around the source and use that?
In the JS engine, we don't have direct access to whereever that source is coming from; we just have a jschar * array. Creating a reference wrapper around it would have to be handled by the browser's bridge to the JS engine. The JS engine would also have to grow some way to notify the browser that the reference can be dropped. Doubtless it's possible; it's just not trivial.

If you're looking for quick'n'dirty hacks, you could try reducing zlib's compression setting, trading memory for speed.
What about memcpy the jschar * coming in?
> Creating a reference wrapper around it would have to be handled by the browser's bridge
> to the JS engine.

For the cases that really matter, the browser already has this in a refcounted heap object, for what it's worth.

> The JS engine would also have to grow some way to notify the browser that the reference
> can be dropped.

For purposes of the browser, just calling a browser-defined callback function with the relevant const jschar* is enough, fwiw.

Note that the browser would need to know whether to wait for such a call or not.
What sort of schedule is this needed on? I'm not too familiar with the B2G release schedule, but I understand tef? means it's wanted for the very-soon Telefonica drop. Even with the fairly simple compression model we have today, there have been subtle threading and reentrancy issues. I would consider any of the nontrivial modifications suggested in this bug risky changes to make to RC phase code.
Benjamin, tef+ bugs are due to land tomorrow evening... I we can get a 1 sec speedup that would be a huge win, but I totally understand if it's too risky to do properly. Would the current patch be acceptable as a short term fix?
If it helps, I think the current patch would be okay. Somebody needs to measure, though. I have no idea if it will.
Benjamin, can you rebase your patch on b2g18? I tried to do it but there are some changes that I was not sure about (mostly complete() instead of ensureReady() in SourceCompressionToken)
I'm attaching a backport for bg18. You will also need the backport on bug 825545.
This is a large perf win, so we'll block on it.
Assignee: general → benjamin
blocking-b2g: tef? → tef+
Initial timings don't show any improvements unfortunately. For example, we're still at 3.5/4.0 second of startup time for the email app.
If compression is taking up > 1 second out of a 4 second startup, it seems like you really don't want to be doing in the first place.  Just guessing here since I don't know anything about how these apps are architected, but I would assume the source is stored somewhere and can be retrieved on demand from memory or disk.
(In reply to Fabrice Desré [:fabrice] from comment #16)
> Initial timings don't show any improvements unfortunately. For example,
> we're still at 3.5/4.0 second of startup time for the email app.

I think you would want to apply the teeny patch in https://bugzilla.mozilla.org/show_bug.cgi?id=817095#c16 to gaia-email-opt.js first to get some overlapped execution.  Right now it uses a setTimeout, which sounds like it's a no-go because of the inability to yield to the event loop.


Is the problem with source compression that zlib is stupid slow on our hardware?  It looks like the compression thread is created with normal priority, so it should not be completely swamped out by other threads.  Is there an optimized version of zlib we can use, or can we switch to snappy like our IndexedDB impl uses?
Keywords: perf
I retested before/after with the gaia-email-opt.js fix, but get the same results.
Looking at the profile, we spend about 50ms in execution after waiting for the source to compress. So the most we'd gain is 50ms, however since we're on a single core processor, I'd be surprised if we saw anything close to that.
Can you link the profile?  The most recent one referenced I can see is:
http://people.mozilla.com/~bgirard/cleopatra/#report=6e54c691510ca5bd0d203a8c5b59013f613c313b

where 117 ticks are spent in a __futex_syscall3 for gaia-email-opt.js as I read it.  (Since it's onStopRequest on the stack, I'm assuming the whole JS file is buffered at that point and there's no other blocking for reasons like that.)
(In reply to Andrew Sutherland (:asuth) from comment #21)
> Can you link the profile?  The most recent one referenced I can see is:
> http://people.mozilla.com/~bgirard/cleopatra/
> #report=6e54c691510ca5bd0d203a8c5b59013f613c313b
> 
> where 117 ticks are spent in a __futex_syscall3 for gaia-email-opt.js as I
> read it.  (Since it's onStopRequest on the stack, I'm assuming the whole JS
> file is buffered at that point and there's no other blocking for reasons
> like that.)

That's the same profile I'm looking at. If you look after the 117 ticks in __futex_syscall3 (waiting for compression) you'll see the 5 ticks (50ms) in js::RunScript.
With the fix applied that I mentioned, the 27 ticks under:
- Timer::Fire
  - JS::CallEvetHandler
    - js::RunScript
      - req/<() @ gaia-email-opt.js:248

should also be running in that same js::RunScript.

Also, will Cleopatra show the the tick interval correctly if people are running with that patch to sample at a 1ms interval instead of 10ms, or it will lie and still say 10ms?
(In reply to Andrew Sutherland (:asuth) from comment #23)
> Also, will Cleopatra show the the tick interval correctly if people are
> running with that patch to sample at a 1ms interval instead of 10ms, or it
> will lie and still say 10ms?

Yes, it should still give the correct interval. It measures the interval using timestamps taken at each tick.
Okay, so then when I'm saying ticks, I actually mean 'ms', because the unlabeled units I'm looking at are actually ms.  So really we're looking at (+27 ms =) 32ms of overlapped execution time, and we'll still be waiting for (117ms - 27ms =) 90ms for compression.
fabrice just straightened me out on IRC; I had faked myself out because of that giant 'process waiting for an app start'.  multiply everything I said by 10.  We should be seeing giant wins in fabrice's run, really.
(In reply to Andrew Sutherland (:asuth) from comment #26)
> fabrice just straightened me out on IRC; I had faked myself out because of
> that giant 'process waiting for an app start'.  multiply everything I said
> by 10.  We should be seeing giant wins in fabrice's run, really.

But we're on a single core, so overlapping execution will actually just slow things down, unless we're waiting for io someplace.
(In reply to Brian Hackett (:bhackett) from comment #17)
> If compression is taking up > 1 second out of a 4 second startup, it seems
> like you really don't want to be doing in the first place.  Just guessing
> here since I don't know anything about how these apps are architected, but I
> would assume the source is stored somewhere and can be retrieved on demand
> from memory or disk.

On B2G our packaged apps live in jar files.  So is source compression just re-compressing the js file we extracted from our jar file?  From the source, it kindof looks like it is doing that.

Can we turn it off/selectively break toString on Function/other?


(In reply to Jeff Muizelaar [:jrmuizel] from comment #27)
> But we're on a single core, so overlapping execution will actually just slow
> things down, unless we're waiting for io someplace.

Yeah; I thought he was using the profiler and looking for the time to move around rather than the overall wall clock which would stay the same, but he was wall-clocking.
(In reply to Andrew Sutherland (:asuth) from comment #28)
> On B2G our packaged apps live in jar files.  So is source compression just
> re-compressing the js file we extracted from our jar file?  From the source,
> it kindof looks like it is doing that.

Yes, it is.

> Can we turn it off/selectively break toString on Function/other?

The attached patch should change the behavior of Function.toString so that it always returns "[sourceless code]" for the body, and will (it looks like; I don't know this code) turn off source compression.  This changes JS behavior observably of course, but maybe things won't break?
This last patch makes a big difference on the email app, indeed (around 1.5 seconds speedup). How safe is that?
(In reply to Fabrice Desré [:fabrice] from comment #30)
> This last patch makes a big difference on the email app, indeed (around 1.5
> seconds speedup). How safe is that?

Clever unit testing/RPC frameworks have been known to use Function.toString to let a function be easily remoted.  For example, selenium/webdriver drivers for JS have used this.

James Lal has indicated that we don't use these tricks in our unit testing frameworks that he's involved in.

Also, in places where CSP is around to stop eval()/new Function(string) from working, I would expect it to be pretty safe.  But that obvious is about the other side of the connection.

I'd be mainly worried about breaking web pages that would expect certain behaviours from SpiderMonkey...
(In reply to Andrew Sutherland (:asuth) from comment #31)

> I'd be mainly worried about breaking web pages that would expect certain
> behaviours from SpiderMonkey...

That's also my concern. I don't mind doing whatever we need in gaia, but breaking tons of sites is a no-no.
Sure, we definitely don't want to do this for content.  What does the stack look like when gaia scripts are parsed?  I'm hoping this can be done selectively for gaia either with a modified JSAPI call, or by bracketing the parsing with some call pair JS_StartDontSaveScriptSource / JS_EndDontSaveScriptSource.
Gaia is content, so I'm not sure what we can do there. 
<script type=application/javascript;no-source> maybe ?
nsIPrincipal.appStatus == nsIPrincipal::APP_STATUS_CERTIFIED for our apps.

The global window can set the compilation options on all the compartments based on this information, I would think.

There's also the strong relationship currently where APP_STATUS_CERTIFIED means it's an app:// protocol backed by a jar:file:// or jar:remoteopenfile:// (and also that CSP is turned up so that eval is forbidden.)
This patch adds APIs for specifying when script source should not be saved.
Please don't tie this to app privilege status.  It's anticompetitive and will hurt the product in the long run if developers can't make their apps as fast as those that happened to preinstalled in a certain way.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #37)
> Please don't tie this to app privilege status.  It's anticompetitive and
> will hurt the product in the long run if developers can't make their apps as
> fast as those that happened to preinstalled in a certain way.

I agree. But the idea of tying that to a csp that forbid eval looks ok to me.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #37)
> Please don't tie this to app privilege status.  It's anticompetitive and
> will hurt the product in the long run if developers can't make their apps as
> fast as those that happened to preinstalled in a certain way.

I also agree overall.  In my mind, the problem is that source compression is way too slow on our hardware right now and it sounds like it's hard or at least scary to overhaul at this point for v1.0.0.  I worry that exposing something like "no-source" could propagate this wild hackjob much further than we would intend and/or raise semantics issue.  We have already made compromises with certified apps that are very arguably anti-competitive; unless you are certified (privileged?), you can't get TCP access and write an IMAP client already anyways.

It seems like maybe a better longer-term solution might be to allow us to operate in a mode where the app:// protocol can just re-provide the source on demand.  There's obviously a sync/async semantics issue there, but perhaps if Function.toString gets invoked, either an evil-nested-event-loop could be spun or just blatant blocking of the event loop.  Once that happens, the script could be latched.  During initial parsing a very naive parsing heuristic could detect something like (function bob() {}.toString()) which would cause us to never discard the source in the first place.  Or we could run the 'no-source' MIME parameter or a 'use no-source' 'use strict'-style directive up the JS/HTML standard flag-pole.
(In reply to Andrew Sutherland (:asuth) from comment #39)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #37)
> > Please don't tie this to app privilege status.  It's anticompetitive and
> > will hurt the product in the long run if developers can't make their apps as
> > fast as those that happened to preinstalled in a certain way.
> 
> we have already made
> compromises with certified apps that are very arguably anti-competitive;
> unless you are certified (privileged?), you can't get TCP access and write
> an IMAP client already anyways.

I also don't disagree with your comment overall, but this part isn't true: third-party privileged apps can get TCP access.
Just throwing it out there, but we could also just make toString on functions, inside any app (privileged or not), return "function name() { [source unavailable] }" and not compress or anything.  There's no reason any of this stuff should actually need function source, there's always a way to avoid it.  *That* change may well be easier than anything else that's practical at this point, although I'm assuming the set of apps out there doesn't use function toString much except for diagnostics.
Doing a couple more easy things should help a lot here.
1. We don't seem to be minifying the gaia source code. Doing this could cut the 1.2mb js in half and should see the compression time improve around the same. This would also help the large amount of time we spend parsing. I've filed bug 837111
2. Switch to a faster compressor. Snappy and lz4 are about 10x faster than gzip. See bug 837110.

In the longer term we can switch to some kind of shared ownership of the js source so that source compression can continue after the return of js::frontend::CompileScript
Comment on attachment 708377 [details] [diff] [review]
Allow source compression to run while executing the script

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

Sorry to be dense here, but I don't understand the existing code in ScriptSource::substring, particularly how it will deal with the case where fun.toString() is racing with the compression thread. It doesn't look safe to me; ScriptSource::ready and SourceCompressorThread::currentChars don't do any locking.

Also please update the comment in SourceCompressorThread::finish that says "We should only be compressing things when in the compiler."

Do we have a test already where we do something that goes through Evaluate, and function.toString() races with compression? I think we want two tests: one where the racing toString call happens during the evaluate(), and one where it happens just after.

Clearing review flag.
Attachment #708377 - Flags: review?(jorendorff)
(In reply to Jason Orendorff [:jorendorff] from comment #43)
> Comment on attachment 708377 [details] [diff] [review]
> Allow source compression to run while executing the script
> 
> Review of attachment 708377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry to be dense here, but I don't understand the existing code in
> ScriptSource::substring, particularly how it will deal with the case where
> fun.toString() is racing with the compression thread. It doesn't look safe
> to me; ScriptSource::ready and SourceCompressorThread::currentChars don't do
> any locking.

It looks scary, but it's actually okay because SourceCompressorThread::tok is cleared by the mainthread. Thus, even if compression is finished

> 
> Also please update the comment in SourceCompressorThread::finish that says
> "We should only be compressing things when in the compiler."

Will do.

> 
> Do we have a test already where we do something that goes through Evaluate,
> and function.toString() races with compression? I think we want two tests:
> one where the racing toString call happens during the evaluate(), and one
> where it happens just after.

We sort of have these. We tests that ask for source while it's definitely in the compression thread. It's, of course, a bit tricky to write tests to reproduce particular race conditions exactly.
Revised comment in ::compress.
Attachment #708377 - Attachment is obsolete: true
Attachment #709875 - Flags: review?(jorendorff)
(In reply to :Benjamin Peterson from comment #44)
> (In reply to Jason Orendorff [:jorendorff] from comment #43)
> > Comment on attachment 708377 [details] [diff] [review]
> > Allow source compression to run while executing the script
> > 
> > Review of attachment 708377 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Sorry to be dense here, but I don't understand the existing code in
> > ScriptSource::substring, particularly how it will deal with the case where
> > fun.toString() is racing with the compression thread. It doesn't look safe
> > to me; ScriptSource::ready and SourceCompressorThread::currentChars don't do
> > any locking.
> 
> It looks scary, but it's actually okay because SourceCompressorThread::tok
> is cleared by the mainthread. Thus, even if compression is finished

SourceCompressorThread::tok will still be valid until the main thread calls waitOnCompression.
Comment on attachment 709875 [details] [diff] [review]
Allow source compression to run while executing the script

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

Right. Looks good.
Attachment #709875 - Flags: review?(jorendorff) → review+
Comment on attachment 709875 [details] [diff] [review]
Allow source compression to run while executing the script

[Approval Request Comment]
Blocking tef+ as you can see.
Attachment #709875 - Flags: approval-mozilla-b2g18?
Comment on attachment 709875 [details] [diff] [review]
Allow source compression to run while executing the script

tef+ blockers already have approval to land.
Attachment #709875 - Flags: approval-mozilla-b2g18?
Blocks: 821040
For what it's worth, this was a 4+% win on Kraken on Tinderbox too...
https://hg.mozilla.org/mozilla-central/rev/71f14579265f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: