Closed Bug 851627 Opened 12 years ago Closed 12 years ago

Make LifoAlloc::release run in O(1)

Categories

(Core :: JavaScript Engine, defect)

x86_64
Windows 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: vlad, Assigned: luke)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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.
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.
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.
Blocks: 854599
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.
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.
Alternatively, and probably much better, we could change the mark/release API so that release had enough data to be O(1).
Summary: OdinMonkey: IonRuntime::initialize taking 2.6s in compileVMFunctions when started with large JS codebase → Make LifoAlloc::release run in O(1)
Attached patch patch (obsolete) (deleted) — Splinter Review
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)
Attached patch patch (qref'd) (deleted) — Splinter Review
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+
(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 :)
But yes, good idea, I'll do that.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: