513276 - TM: make Assembler short-lived

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect, P2)

Description

[Andreas Gal :gal]

With the CodeAlloc patches landed there is no need to keep Assemblers around. It should be stack allocated in TR::compile(). Also, we should really have a tempAlloc in TM and use that for everything the assembler allocates. The current allocator should become lirAlloc.

Comment 1

[Andreas Gal :gal]

If we take the new allocator code for fennec/1.9.2, we will make memory use slightly worse for the code cache until we fix this bug (slightly, probably not huge).

Comment 2

[Stuart Parmenter]

Andreas: any idea what the impact here is?

Comment 3

[Andreas Gal :gal]

We don't have any numbers on the overhead. There are two problems here, and we can chose to fix one or both:

1) we use slightly more memory than we used to with the old allocator because temporary allocations go into the long-lived allocator. We need a temporary allocator instead.

2) we still keep the LIR around. We should stop doing that, except when we really need it.

I will try to finish 1) before the fennec beta, or at least get some numbers on how much we lose if we don't have it.

Comment 4

[Andreas Gal :gal]

Graydon is working on the bugs this depends on. Once that is done this one is trivial.

Comment 5

[Damon Sicore (:damons)]

Blocking 1.9.2+.  P2.

Comment 6

[Andreas Gal :gal]

Graydon, assigning this to you. Happy to help though and take the bug myself. Lets coordinate.

Comment 7

[Graydon Hoare :graydon]

Attachment: Do the deed

The attached patch "makes assembler short-lived".

It does *work*, but I'm actually going to nominate this bug for closing WONTFIX. It's not important given the general gutting of nanojit components and component lifecycles, the "short-lived" assembler code is actually a touch uglier than the long-lived, and worst, it regresses performance due to all the assembler setup/teardown noise (not a lot, about 5ms on sunspider).

This is unrelated, also, to the work-in-progress to lift LIR out into its own recording-lifecycle allocator.

Comment 8

[Luke Wagner [:luke]]

(In reply to comment #7)
> Created an attachment (id=402462) [details]
> Do the deed
> 
> The attached patch "makes assembler short-lived".
> 
> It does *work*, but I'm actually going to nominate this bug for closing
> WONTFIX. It's not important given the general gutting of nanojit components and
> component lifecycles, the "short-lived" assembler code is actually a touch
> uglier than the long-lived, and worst, it regresses performance due to all the
> assembler setup/teardown noise (not a lot, about 5ms on sunspider).
> 
> This is unrelated, also, to the work-in-progress to lift LIR out into its own
> recording-lifecycle allocator.

Talking to dvander (and from a quick perusal), it looks like Assembler::patch could be made into a non-member function.  This would remove the need for constructing an Assembler object just to call 'patch'.  I'm not sure how often JoinPeers is called, but perhaps that would improve perf?  Further simplfying the resulting code, nanojit::compiler (or a wrapper thereof) could construct the Assembler, thereby removing nanojit::Assembler from jstracer altogether.

Comment 9

[Graydon Hoare :graydon]

A valid point. I can rework the patch to approach it this way, if you like. I'd prefer to do that after the NJ merge completes, if that's ok. I think this was initially motivated by performance concerns regarding retained allocations (if you read the bug description), and we've addressed almost all of those elsewhere via independent work on moving allocations to allocators with appropriate lifecycles.

The remaining difference is just a bit of style / housecleaning. Perhaps worth doing, but not taking priority just now.

I'd at very least blocking1.9.2- this.

Comment 10

[Luke Wagner [:luke]]

(In reply to comment #9)
> The remaining difference is just a bit of style / housecleaning. Perhaps worth
> doing, but not taking priority just now.

Totally agreed, I just hated to see the idea go away.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Obsolete with the removal of tracejit.