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

RESOLVED DUPLICATE of bug 898963

Status

()

Core
JavaScript Engine
RESOLVED DUPLICATE of bug 898963
5 years ago
4 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 766788 [details] [diff] [review]
Patch
Assignee: general → hv1989
Attachment #766788 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Blocks: 882399

Comment 2

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

Comment 3

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

5 years ago
Comment on attachment 766788 [details] [diff] [review]
Patch

r+ using ReturnReg, no push/pop, and JS_ASSERT not equal JSReturnReg_Data/Type.
Attachment #766788 - Flags: review?(luke) → review+
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.
Blocks: 896808
(Assignee)

Comment 6

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

5 years ago
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 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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 898963
You need to log in before you can comment on or make changes to this bug.