997274 - Value.h should use NUNBOX32 and PUNBOX64 instead of JS_BITS_PER_WORD

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently macro assembler backends are using either NUNBOX32 or PUNBOX64, and this are the name of the Value representations.

The rest of the engine do not care much about which value representation is used, except for speed issues.  But most of all, the value representation defined in Value.h *must be* in sync with what the Macro Assembler expect to handle.

As Jason pointed on IRC, these defines are not used in Value.h, we should change that.

The only issue is that these macro are only known by the JS engine, and not exported through the JS API, as opposed to Value.h.

We should remove these JS_NUNBOX32 and JS_PUNBOX64 from the configure and move them to Value.h or Value-config.h or something similar where we can make this choice globally.

Comment 1

[hev [:hev]]

MIPS N32 has 32-bit pointers and 64-bit register, so JS_BITS_PER_WORD == 4. The problem is PUNBOX64 can works for it, NUNBOX32 get only in current code.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Heiher from comment #1)
> MIPS N32 has 32-bit pointers and 64-bit register, so JS_BITS_PER_WORD == 4.
> The problem is PUNBOX64 can works for it, NUNBOX32 get only in current code.

This is exactly why I would be interested in giving you the opportunity to look into this bug, if you don't mind?
JS_BITS_PER_WORD is does not have a clear meaning, and we want to remove it.

I do not know much about MIPS N32, but I guess NUNBOX32 remains the most efficient for boxing for it, as long as you have a cheap instruction for extracting the low 32 bits of a register.

Comment 3

[David]

(In reply to Nicolas B. Pierron [:nbp] from comment #0)

Hi, I am a beginner. I want to give it a try.

Could you help me to get start with debugging and become my mentor?

Comment 4

[Nicolas B. Pierron [:nbp]]

David

Comment 5

[Nicolas B. Pierron [:nbp]]

Hi David,

(In reply to David from comment #3)
> Hi, I am a beginner. I want to give it a try.

Sorry for the previous message. Sure, I'll add you as assignee on this issue.

(In reply to David from comment #3)
> Could you help me to get start with debugging and become my mentor?

You can look at
https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey

This way you will get a running JavaScript shell and you would be able to test your modifications.

Value.h is located in js/public/Value.h

Currently, the configure script of the shell defines the NUNBOX32 and PUNBOX64 macro which are used in the JS Engine.  We need to move this computation out-side the configure script.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> Currently, the configure script of the shell defines the NUNBOX32 and
> PUNBOX64 macro which are used in the JS Engine.  We need to move this
> computation out-side the configure script.

You will find the configure script in js/src/configure.in.  Inside this file, we have m4 macro, such as:

    AC_DEFINE(JS_NUNBOX32)
    AC_DEFINE(JS_PUNBOX64)

and the logic which is using these macro should move to a C++ header, included by Value.h

Comment 7

[David]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
Thank you for your resources!

It is quiet late in my country and I am quiet tired today

I will try it tomorrow.

Comment 8

[hev [:hev]]

Attachment: Define-Value-boxing-v1.patch

The patch changes:
1. Replace '#if JS_BITS_PER_WORD == 32' with '#if defined(JS_NUNBOX32)' and PUNBOX64 in js/public/Value.h
2. Add 'JS_NUNBOX32' and 'JS_PUNBOX64' definitions in js/src/js-config.h.in, That will export the Value boxing type in js-config.h and used by Value.h

For specific target platform need select a boxing type in configure.in, That not changed. e.g.:
mips64*-*)
    AC_DEFINE(JS_CPU_MIPS)
    AC_DEFINE(JS_PUNBOX64)  # or JS_NUNBOX32
    ;;
mips*-*)
    AC_DEFINE(JS_CPU_MIPS)
    AC_DEFINE(JS_NUNBOX32)
    ;;

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8408708 [details] [diff] [review]
Define-Value-boxing-v1.patch

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

Does the browser still compiles fine with these changes?
I do not think the browser defines the PUNBOX and NUNBOX macros.

Comment 10

[hev [:hev]]

(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> Comment on attachment 8408708 [details] [diff] [review]
> Define-Value-boxing-v1.patch
> 
> Review of attachment 8408708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does the browser still compiles fine with these changes?
> I do not think the browser defines the PUNBOX and NUNBOX macros.

Hello,

There have two compile systems? one is js/src/configure* and another is moz.build? Do you mean the patch can't works for moz.build? I will check it now.

Thanks!

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Heiher from comment #10)
> (In reply to Nicolas B. Pierron [:nbp] from comment #9)
> > Comment on attachment 8408708 [details] [diff] [review]
> > Define-Value-boxing-v1.patch
> > 
> > Review of attachment 8408708 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Does the browser still compiles fine with these changes?
> > I do not think the browser defines the PUNBOX and NUNBOX macros.
> 
> Hello,
> 
> There have two compile systems? one is js/src/configure* and another is
> moz.build? Do you mean the patch can't works for moz.build? I will check it
> now.

We have 2 different configure scripts.  I think that the configure script in js/src/ is executed by the top-level one, in which case this should be fine.

Comment 12

[Nicolas B. Pierron [:nbp]]

Hi David,

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> (In reply to David from comment #3)
> > Hi, I am a beginner. I want to give it a try.
> 
> Sorry for the previous message. Sure, I'll add you as assignee on this issue.

Let me find you another bug which would be good for starting in the JS Engine.

Comment 13

[hev [:hev]]

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> (In reply to Heiher from comment #10)
> > (In reply to Nicolas B. Pierron [:nbp] from comment #9)
> > > Comment on attachment 8408708 [details] [diff] [review]
> > > Define-Value-boxing-v1.patch
> > > 
> > > Review of attachment 8408708 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Does the browser still compiles fine with these changes?
> > > I do not think the browser defines the PUNBOX and NUNBOX macros.
> > 
> > Hello,
> > 
> > There have two compile systems? one is js/src/configure* and another is
> > moz.build? Do you mean the patch can't works for moz.build? I will check it
> > now.
> 
> We have 2 different configure scripts.  I think that the configure script in
> js/src/ is executed by the top-level one, in which case this should be fine.

Hello,

I have checked compile it in firefox browser and js standalone, Not found compile issue.

Comment 14

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8408708 [details] [diff] [review]
Define-Value-boxing-v1.patch

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

Add an include of js-config.h in Value.h, as done in TypeDecls.h
Otherwise this sounds good. :)

Comment 15

[hev [:hev]]

Attachment: Define-Value-boxing-v2.patch

Comment 16

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8409269 [details] [diff] [review]
Define-Value-boxing-v2.patch

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

Sorry for the delay.

This sounds good, I will push it to our try server, to make sure this works well on all architectures.

Comment 17

[Nicolas B. Pierron [:nbp]]

https://tbpl.mozilla.org/?tree=Try&rev=cc6de6873810

Comment 18

[Nicolas B. Pierron [:nbp]]

Tomcat|sheriffduty> seems you hit on windows a bad slave (bug 853605) and B2G JB Emulator Debug seems to be a git problem :) and the B2G KK Emulator Debug are just timing out a lot :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/76c5ba501d68

Comment 19

[Nicolas B. Pierron [:nbp]]

Sorry for the delay again, I forgot the patch in my patch queue, after seeing the errors on Try.

The patch is now going on inbound and you can follow it on our continuous integration with the following link:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&showall=1&rev=76c5ba501d68

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76c5ba501d68