Make LifoAlloc::release run in O(1)

RESOLVED FIXED in mozilla23

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vlad, Assigned: luke)

Tracking

unspecified
mozilla23
x86_64
Windows 8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.

Updated

5 years ago
Summary: IonRuntime::initialize taking 2.6s in compileVMFunctions when started with large JS codebase → OdinMonkey: IonRuntime::initialize taking 2.6s in compileVMFunctions when started with large JS codebase
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.
Created attachment 725804 [details]
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.
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?
Nope, exactly that same way, on win64.  Huh.
(Assignee)

Comment 5

5 years ago
I saw the this issue on my MBP.  I think it was even from the shell, but I could be wrong.
Hannes, were you doing a 64-bit build?  Both luke and I were, sounds like.
Yep, 64bit, linux build
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).
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.
And thanks for providing the build :D.
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.
(Assignee)

Updated

5 years ago
Blocks: 854599
(Assignee)

Comment 12

5 years ago
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.
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

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

Updated

5 years ago
Summary: OdinMonkey: IonRuntime::initialize taking 2.6s in compileVMFunctions when started with large JS codebase → Make LifoAlloc::release run in O(1)
(Assignee)

Comment 15

5 years ago
Created attachment 739796 [details] [diff] [review]
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.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #739796 - Flags: review?(sstangl)
(Assignee)

Comment 16

5 years ago
Created attachment 739799 [details] [diff] [review]
patch (qref'd)

qref'd
Attachment #739796 - Attachment is obsolete: true
Attachment #739796 - Flags: review?(sstangl)
Attachment #739799 - Flags: review?(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.
Attachment #739799 - Flags: review?(sstangl) → review+
(Assignee)

Comment 18

5 years ago
(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 :)
(Assignee)

Comment 19

5 years ago
But yes, good idea, I'll do that.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 416717
https://hg.mozilla.org/mozilla-central/rev/0ae79e3f9e6f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.