Closed
Bug 1136898
Opened 10 years ago
Closed 10 years ago
IonMonkey: remove LinearScan
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: sunfish, Assigned: sunfish)
References
Details
Attachments
(2 files)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
With the backtracking allocator now enabled by default (bug 826741), we can remove the LinearScan allocator. This also allows us to remove LLabel, LNop, and even LMop (bug 1096138), as well as simplify the liveness computation code in LiveRangeAllocator.cpp.
Attachment #8569416 -
Flags: review?(jdemooij)
Comment 1•10 years ago
|
||
This is awesome, but I think it'd be good to wait at least until next week so we can switch back if we find some serious issues on Nightly. I can start reviewing the patch though.
Comment 2•10 years ago
|
||
Random naming suggestion: since there would no longer be two (serious) allocators, could we globally rename "BacktrackingAllocator" to just "RegisterAllocator" (keeping StupidAllocator as is)? I'm guessing we won't be landing any new experimental allocators for a while...
Comment 3•10 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #2)
> Random naming suggestion: since there would no longer be two (serious)
> allocators, could we globally rename "BacktrackingAllocator" to just
> "RegisterAllocator" (keeping StupidAllocator as is)? I'm guessing we won't
> be landing any new experimental allocators for a while...
There is already a RegisterAllocator class which has code common to the backtracking and stupid allocators. I think after this lands we should fold LiveRangeAllocator and its template mumbo jumbo into BacktrackingAllocator and remove LiveRangeAllocator, but it would be kind of nice to keep the name for BacktrackingAllocator to isolate its function from the more generic stuff.
Comment 5•10 years ago
|
||
Comment on attachment 8569416 [details] [diff] [review]
remove-lsra.patch
Review of attachment 8569416 [details] [diff] [review]:
-----------------------------------------------------------------
Beautiful, removes even more code than I expected :) Please wait until Monday or so, in case BT happens to be crashy on some popular websites.
::: js/src/jit/CodeGenerator.cpp
@@ -4283,5 @@
> iter++;
>
> while (true) {
> for (; iter != block->end(); iter++) {
> - if (iter->isNop() || iter->isConstant() || iter->isPostWriteBarrier()) {
We're checking the MIR graph here so we should keep the iter->isNop() check. MNops are still inserted in some cases.
Attachment #8569416 -
Flags: review?(jdemooij) → review+
Comment 6•10 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #1)
> This is awesome, but I think it'd be good to wait at least until next week
> so we can switch back if we find some serious issues on Nightly. I can start
> reviewing the patch though.
I thought we would keep it for 1/2 months, making sure that if something serious is thrown our way, we have the option to disable it when it is already in aurora/beta.
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #6)
> (In reply to Jan de Mooij [:jandem] from comment #1)
> > This is awesome, but I think it'd be good to wait at least until next week
> > so we can switch back if we find some serious issues on Nightly. I can start
> > reviewing the patch though.
>
> I thought we would keep it for 1/2 months, making sure that if something
> serious is thrown our way, we have the option to disable it when it is
> already in aurora/beta.
How about this: if something serious is thrown our way, and we can't fix it in backtracking, I volunteer to port LinearScan up to trunk and re-land it.
Comment 8•10 years ago
|
||
Rebased patch. It would be cool if this could land soon, as I'd like to do this before bug 1067610 so that bug 1067610 can have a cleaner patch that restructures the backtracking allocator more fundamentally.
Assignee | ||
Comment 9•10 years ago
|
||
jandem, are you ok with this rebased patch landing at this time?
Flags: needinfo?(jdemooij)
Comment 10•10 years ago
|
||
(In reply to Dan Gohman [:sunfish] from comment #9)
> jandem, are you ok with this rebased patch landing at this time?
Seems fine, thanks for doing this!
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•