886411 - OdinMonkey: Remove EnableActivation/DisableActivation functions and do it in assembly

Status: RESOLVED DUPLICATE of bug 898963

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

To finally be able to land, this part was written in c++. It is easy to port it to assembly and removes some overhead.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 766788 [details] [diff] [review]
Patch

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

::: js/src/ion/AsmJS.cpp
@@ -5744,5 @@
> -    LoadAsmJSActivationIntoRegister(masm, callee);
> -    masm.push(scratch);
> -    masm.setupUnalignedABICall(1, scratch);
> -    masm.passABIArg(callee);
> -    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, EnableActivation));

Could you also rm EnableActivation/DisableActivation?

@@ +5758,5 @@
>  
> +    masm.push(callee);
> +    masm.loadJitActivation(callee);
> +    masm.store32(Imm32(0), Address(callee, Activation::offsetOfActive()));
> +    masm.pop(callee);

It seems like 'callee' isn't used after (unless it is JSReturnReg_Type? in which case perhaps you could use ABIArgGenerator::NonArgReturnVolatileReg0?) so you could avoid the push/pop.

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 766788 [details] [diff] [review]
> Patch
> 
> Review of attachment 766788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/AsmJS.cpp
> @@ -5744,5 @@
> > -    LoadAsmJSActivationIntoRegister(masm, callee);
> > -    masm.push(scratch);
> > -    masm.setupUnalignedABICall(1, scratch);
> > -    masm.passABIArg(callee);
> > -    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, EnableActivation));
> 
> Could you also rm EnableActivation/DisableActivation?

Yeah definitely, sorry forgot.


> @@ +5758,5 @@
> >  
> > +    masm.push(callee);
> > +    masm.loadJitActivation(callee);
> > +    masm.store32(Imm32(0), Address(callee, Activation::offsetOfActive()));
> > +    masm.pop(callee);
> 
> It seems like 'callee' isn't used after (unless it is JSReturnReg_Type? in
> which case perhaps you could use ABIArgGenerator::NonArgReturnVolatileReg0?)
> so you could avoid the push/pop.

ABIArgGenerator::NonArgReturnVolatileReg0 = ecx = JSReturnReg_Type
ABIArgGenerator::NonArgReturnVolatileReg1 = edx = JSReturnReg_Data

So both need to be preserved.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 766788 [details] [diff] [review]
Patch

r+ using ReturnReg, no push/pop, and JS_ASSERT not equal JSReturnReg_Data/Type.

Comment 5

[Douglas Crosher [:dougc]]

EnableActivation and DisableActivation are showing up a relatively 'hot' functions in profiling work. For example, 1.74% and 1.31% respectively of libxul activity, which puts them in the top 6 of libxul functions, so this might be a useful improvement.

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Douglas Crosher [:dougc] from comment #5)
> EnableActivation and DisableActivation are showing up a relatively 'hot'
> functions in profiling work. For example, 1.74% and 1.31% respectively of
> libxul activity, which puts them in the top 6 of libxul functions, so this
> might be a useful improvement.

Thanks for reconfirming. I had a lot of fail-out related to EnableActivation/DisableActivation. Therefore I was just happy when it finally worked. The patch is also incorrect atm :(.

To get everything fixed I possible did a bit to much in EnableActivation/DisableActivition too. Normally not everything there should be needed. So I should first start to weed out the unneeded functions, before I can put it in assembly again.

I'm sad to say this, but I don't have the time to look at this the coming week. It might make sense to ping me about this late next week...

Comment 7

[Luke Wagner [:luke]]

ping :)  I have a new (albeit small) reason to want this: bug 900669 needs to avoid all AbsoluteAddress creation on asm.js compilation paths and callWithABI uses AbsoluteAddress.

Comment 8

[Dan Gohman [:sunfish]]

Comment on attachment 766788 [details] [diff] [review]
Patch

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

::: js/src/ion/AsmJS.cpp
@@ +5758,5 @@
>  
> +    masm.push(callee);
> +    masm.loadJitActivation(callee);
> +    masm.store32(Imm32(0), Address(callee, Activation::offsetOfActive()));
> +    masm.pop(callee);

The "scratch" register appears available here, for avoiding the push/pop.