851627 - Make LifoAlloc::release run in O(1)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

If the large Asm.js GDC demo codebase is the first thing to trigger IonRuntime::initialize, it takes 2600ms to execute.  The profiler is claiming that all the time is in Maybe::~Maybe but I don't know if I trust that.  If something like sunspider, or a much smaller Ion codebase triggers it, initialize takes around 120ms.  (The times were verified using a printf around intialize(), with JS_Now to make sure the profiler times were correct.)

Luke tells me that he saw this as well and wasn't sure what's going on.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Sorry, I was wrong about the smaller codebase above.  When Asm.js is being used, it takes a lot more.  If it's not, then it's pretty much 120ms.

(The much smaller codebase I mentioned above wasn't actually using asm.js.)

If I try with a simple hello world using asm.js, initialize takes around 370-400ms.  If I change "use asm"; to "zuse asm"; or similar, it goes back to 120ms.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: the simple Asm.js testcase I Was using

Add some timing printfs in initialize(), and you should see a big difference if this page is the first one that you load in a browser, or if you load something non-asm.

Comment 3

[Hannes Verschore [:h4writer]]

I tried this over the weekend, but I couldn't reproduce. I created a normal build on the last mozilla-central (that includes asm.js). Using make -f client.mk. And I measured the time difference using "clock()" and took the diff to 1ms certainty. I added the start, just after the { and stop just before "return true;" I used the testcase provided in here, but on both "use asm" and "use bogus" I saw the same init'ing time. Are you measuring this in a different way?

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Nope, exactly that same way, on win64.  Huh.

Comment 5

[Luke Wagner [:luke]]

I saw the this issue on my MBP.  I think it was even from the shell, but I could be wrong.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hannes, were you doing a 64-bit build?  Both luke and I were, sounds like.

Comment 7

[Hannes Verschore [:h4writer]]

Yep, 64bit, linux build

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://people.mozilla.com/~vladimir/misc/firefox-22.0a1.en-US.win64-x86_64.zip

Note, if it doesn't print anything, you need to run "editbin /subsystem:console firefox.exe".  This is a recent nightly build with a few patches applied; only JS one that is relevant is sstangl's patch from bug 850070 (parallel asm.js ion compilation).

Comment 9

[Hannes Verschore [:h4writer]]

With this build I can definitely see the discrepancy. 200ms vs 600ms. When I was testing I didn't apply bug 850070. I'll see tomorrow if that could be the reason I don't see it on linux.

Comment 10

[Hannes Verschore [:h4writer]]

And thanks for providing the build :D.

Comment 11

[Hannes Verschore [:h4writer]]

I updates to a new build (there were some IonRuntime related changes) + added this parallel execution patch. Still couldn't reproduce on linux. I tried to make a build myself on windows, but failed to install the Directx SDK.

Comment 12

[Luke Wagner [:luke]]

I'm seeing this in the profiler again and the time in IonRuntime::initialize is all in ~MacroAssembler under all the IonRuntime::generateVMWrapper calls.  I'm pretty sure this is another instance of the "LifoAlloc::release is O(n) where n is the size of memory allocated in the LifoAlloc" problem; in this case n is 1.5GiB.

Hopefully simple fix.

Comment 13

[Luke Wagner [:luke]]

Yes, that's what's happening, if there is no live TempAllocator, MacroAssembler() creates an AutoIonContextAlloc which reuses cx->tempLifoAlloc() (which the parser also uses).  To remove this footgun, I think we should change TempAllocator to not call mark/release, remove AutoIonContextAlloc, and always create a new LifoAlloc (which we already almost do) so that we don't have to release(), we just delete the LifoAlloc.

Comment 14

[Luke Wagner [:luke]]

Alternatively, and probably much better, we could change the mark/release API so that release had enough data to be O(1).

Comment 15

[Luke Wagner [:luke]]

Attachment: patch

We should have done this *ages* ago.

In an opt shell, this takes the compilation time of Citadel from 16.2s to 12.36s.

Comment 16

[Luke Wagner [:luke]]

Attachment: patch (qref'd)

qref'd

Comment 17

[Sean Stangl [:sstangl]]

Comment on attachment 739799 [details] [diff] [review]
patch (qref'd)

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

r+ with removal of markSlow() and releaseSlow().

::: js/src/jsdbgapi.cpp
@@ +478,5 @@
>      if (!FillBindingVector(script, &bindings))
>          return NULL;
>  
>      /* Munge data into the API this method implements.  Avert your eyes! */
> +    *markp = cx->tempLifoAlloc().markSlow();

Gross. Baking LifoAlloc internal mark pointers into the public API was an interesting choice.

We could work around the API in an amusing way that would let us entirely remove markSlow() and releaseSlow():

On entry to this function, grab a cx->tempLifoAlloc().mark(), then allocate sizeof(LifoAlloc::Mark) bytes into the cx->tempLifoAlloc() itself, and store the Mark there. If we then return the address of the stored Mark, JS_ReleaseFunctionLocalNameArray() can call release() after a cast.

This is assuming that the markp is intended to be opaque, which, boy, I sure do hope is the case.

Comment 18

[Luke Wagner [:luke]]

(In reply to Sean Stangl [:sstangl] from comment #17)
> We could work around the API in an amusing way that would let us entirely
> remove markSlow() and releaseSlow():

I prefer to remove the entire JSAPI with jsd :)

Comment 19

[Luke Wagner [:luke]]

But yes, good idea, I'll do that.

Comment 20

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae79e3f9e6f

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ae79e3f9e6f