Bug 160370 – [JSC] Simplify the initialization of AbstractValue in the AbstractInterpreter

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

[JSC] Simplify the initialization of AbstractValue in the AbstractInterpreter

[Benjamin Poulain]

Created attachment 284927 [details]
Patch

[Benjamin Poulain]

This removes a lot of code from AI but it is unfortunately neutral on my machine:

                                               Conf#1                    Conf#2                                      
SunSpider:
   3d-cube                                 4.6607+-0.0570     ?      4.7053+-0.0694        ?
   3d-morph                                4.9754+-0.0189     ?      4.9997+-0.0682        ?
   3d-raytrace                             4.9289+-0.0512            4.8952+-0.0288        
   access-binary-trees                     1.9846+-0.0223            1.9577+-0.0177          might be 1.0137x faster
   access-fannkuch                         6.0715+-0.1012     ^      5.7134+-0.0343        ^ definitely 1.0627x faster
   access-nbody                            2.3853+-0.0301            2.3680+-0.0180        
   access-nsieve                           2.9449+-0.0209            2.9354+-0.0185        
   bitops-3bit-bits-in-byte                1.0668+-0.0152     ?      1.0778+-0.0155        ? might be 1.0103x slower
   bitops-bits-in-byte                     2.5943+-0.0213            2.5793+-0.0169        
   bitops-bitwise-and                      2.0036+-0.0239            2.0009+-0.0236        
   bitops-nsieve-bits                      3.0817+-0.0104     ?      3.0856+-0.0118        ?
   controlflow-recursive                   2.3074+-0.0138            2.2908+-0.0127        
   crypto-aes                              4.4115+-0.0274     ?      4.4317+-0.0225        ?
   crypto-md5                              2.6880+-0.0325            2.6736+-0.0548        
   crypto-sha1                             2.7485+-0.0254            2.7223+-0.0172        
   date-format-tofte                       6.4954+-0.0599            6.3953+-0.0442          might be 1.0157x faster
   date-format-xparb                       4.7952+-0.1289            4.7482+-0.0364        
   math-cordic                             2.7721+-0.0600            2.7366+-0.0179          might be 1.0130x faster
   math-partial-sums                       4.0024+-0.0273            3.9921+-0.0361        
   math-spectral-norm                      2.0450+-0.0270            2.0450+-0.0252        
   regexp-dna                              6.3581+-0.0782            6.3234+-0.0645        
   string-base64                           4.0282+-0.0512            3.9801+-0.0235          might be 1.0121x faster
   string-fasta                            5.4896+-0.0258     ?      5.5056+-0.0268        ?
   string-tagcloud                         8.3279+-0.0888            8.3228+-0.0722        
   string-unpack-code                     18.2259+-0.1800     ?     18.2791+-0.1459        ?
   string-validate-input                   4.0653+-0.0221     ?      4.1522+-0.1349        ? might be 1.0214x slower

   <arithmetic>                            4.4407+-0.0114            4.4199+-0.0119          might be 1.0047x faster

                                               Conf#1                    Conf#2                                      
Kraken:
   ai-astar                                87.836+-0.724             87.421+-0.691         
   audio-beat-detection                    38.855+-0.089             38.788+-0.083         
   audio-dft                               97.304+-0.574      ?      98.141+-0.849         ?
   audio-fft                               30.213+-0.025      ?      30.299+-0.141         ?
   audio-oscillator                        48.174+-0.391             47.817+-0.148         
   imaging-darkroom                        61.263+-0.144      ?      61.289+-0.203         ?
   imaging-desaturate                      44.790+-0.457             44.496+-0.380         
   imaging-gaussian-blur                   63.573+-1.080      ?      64.037+-1.111         ?
   json-parse-financial                    34.275+-0.304      ?      34.292+-0.404         ?
   json-stringify-tinderbox                23.362+-0.194      !      23.862+-0.210         ! definitely 1.0214x slower
   stanford-crypto-aes                     38.457+-2.741             37.150+-0.318           might be 1.0352x faster
   stanford-crypto-ccm                     33.446+-0.730      ?      34.270+-0.420         ? might be 1.0247x slower
   stanford-crypto-pbkdf2                  93.813+-0.649             93.456+-0.527         
   stanford-crypto-sha256-iterative        30.351+-0.139      ?      30.515+-0.288         ?

   <arithmetic>                            51.837+-0.228      ?      51.845+-0.129         ? might be 1.0002x slower

                                               Conf#1                    Conf#2                                      
Geomean of preferred means:
   <scaled-result>                         4.7977+-0.0111            4.7869+-0.0106          might be 1.0022x faster

[Benjamin Poulain]

Created attachment 284928 [details]
Patch

[Saam Barati]

Comment on attachment 284928 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 284928 [details]
Patch

Clearing flags on attachment: 284928

Committed r204065: <http://trac.webkit.org/changeset/204065>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Comment on attachment 284928 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284928&action=review

The build is broken due to this assert on Apple Windows and Arm Linux bots, see build.webkit.org.

> Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:37
> +static_assert(sizeof(AbstractValue) == (sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue)), "AbstractValue should be as small as possible.");

.

[Benjamin Poulain]

(In reply to comment #7)
> Comment on attachment 284928 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284928&action=review
> 
> The build is broken due to this assert on Apple Windows and Arm Linux bots,
> see build.webkit.org.
> 
> > Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:37
> > +static_assert(sizeof(AbstractValue) == (sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue)), "AbstractValue should be as small as possible.");
> 
> .

http://trac.webkit.org/changeset/204077

Any idea why the assertion does not hold?

[Saam Barati]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 284928 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=284928&action=review
> > 
> > The build is broken due to this assert on Apple Windows and Arm Linux bots,
> > see build.webkit.org.
> > 
> > > Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:37
> > > +static_assert(sizeof(AbstractValue) == (sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue)), "AbstractValue should be as small as possible.");
> > 
> > .
> 
> http://trac.webkit.org/changeset/204077
> 
> Any idea why the assertion does not hold?

This seems quite peculiar. Is the compiler adding padding for some reason somewhere?
I don't understand why it would.

[Csaba Osztrogonác]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 284928 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=284928&action=review
> > 
> > The build is broken due to this assert on Apple Windows and Arm Linux bots,
> > see build.webkit.org.
> > 
> > > Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:37
> > > +static_assert(sizeof(AbstractValue) == (sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue)), "AbstractValue should be as small as possible.");
> > 
> > .
> 
> http://trac.webkit.org/changeset/204077
> 
> Any idea why the assertion does not hold?

Thanks for the quick fix. I checked it on ARMv7 Linux and got the following:
- sizeof(AbstractValue) = 24
- sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue) = 20 (4 + 8 + 8)

[Benjamin Poulain]

(In reply to comment #10)
> > Any idea why the assertion does not hold?
> 
> Thanks for the quick fix. I checked it on ARMv7 Linux and got the following:
> - sizeof(AbstractValue) = 24
> - sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue) = 20 (4 + 8 + 8)

Shit, the JSValue is aligned on 8 bytes!
Thanks Ossy! I'll follow up with a separate patch to fix this.

[Saam Barati]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Comment on attachment 284928 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=284928&action=review
> > > 
> > > The build is broken due to this assert on Apple Windows and Arm Linux bots,
> > > see build.webkit.org.
> > > 
> > > > Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:37
> > > > +static_assert(sizeof(AbstractValue) == (sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue)), "AbstractValue should be as small as possible.");
> > > 
> > > .
> > 
> > http://trac.webkit.org/changeset/204077
> > 
> > Any idea why the assertion does not hold?
> 
> Thanks for the quick fix. I checked it on ARMv7 Linux and got the following:
> - sizeof(AbstractValue) = 24
> - sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue) = 20 (4 + 8 + 8)

I suspect 4 bytes of padding is being added between the two unsigneds and the JSValue
to make the JSValue 8-byte aligned. It's just peculiar that the compiler would do this
on 32-bit platforms. Maybe I'm missing some restriction that requires this.

[Saam Barati]

(In reply to comment #11)
> (In reply to comment #10)
> > > Any idea why the assertion does not hold?
> > 
> > Thanks for the quick fix. I checked it on ARMv7 Linux and got the following:
> > - sizeof(AbstractValue) = 24
> > - sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue) = 20 (4 + 8 + 8)
> 
> Shit, the JSValue is aligned on 8 bytes!
> Thanks Ossy! I'll follow up with a separate patch to fix this.

Do you know why this is needed though on 32-bit platforms?

[Csaba Osztrogonác]

offsetof(AbstractValue, m_structure) == 0   sizeof(StructureAbstractValue) == 4
offsetof(AbstractValue, m_type) == 4        sizeof(SpeculatedType) == 4
offsetof(AbstractValue, m_arrayModes) == 8  sizeof(ArrayModes) == 4
offsetof(AbstractValue, m_value) == 16      sizeof(JSValue) == 8

sizeof(JSValue) is 8, the compiler should add 4 bytes padding before it.

[Csaba Osztrogonác]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > > Any idea why the assertion does not hold?
> > > 
> > > Thanks for the quick fix. I checked it on ARMv7 Linux and got the following:
> > > - sizeof(AbstractValue) = 24
> > > - sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue) = 20 (4 + 8 + 8)
> > 
> > Shit, the JSValue is aligned on 8 bytes!
> > Thanks Ossy! I'll follow up with a separate patch to fix this.
> 
> Do you know why this is needed though on 32-bit platforms?

AFAIK the natural alignment is mandatory everywhere.
Isn't it true on Apple platforms?

I think the easiest fix would be to put the JSValue member 
to the begginning of the struct to avoid padding.

[Saam Barati]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > > Any idea why the assertion does not hold?
> > > > 
> > > > Thanks for the quick fix. I checked it on ARMv7 Linux and got the following:
> > > > - sizeof(AbstractValue) = 24
> > > > - sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue) = 20 (4 + 8 + 8)
> > > 
> > > Shit, the JSValue is aligned on 8 bytes!
> > > Thanks Ossy! I'll follow up with a separate patch to fix this.
> > 
> > Do you know why this is needed though on 32-bit platforms?
> 
> AFAIK the natural alignment is mandatory everywhere.
> Isn't it true on Apple platforms?
> 
> I think the easiest fix would be to put the JSValue member 
> to the begginning of the struct to avoid padding.

So if I have a member that is 1000 bytes in size, it needs to be aligned
on a 1000 byte marker?

I guess in my brain I always thought alignment mattered up to modulo platform pointer
size. That's why I would have guessed that the JSValue could be 4-byte aligned on
32-bit platforms.

Anyways, I agree that we should move the JSValue to be the first element in the class.

[Benjamin Poulain]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > > Any idea why the assertion does not hold?
> > > > 
> > > > Thanks for the quick fix. I checked it on ARMv7 Linux and got the following:
> > > > - sizeof(AbstractValue) = 24
> > > > - sizeof(void*) + sizeof(unsigned) * 2 + sizeof(JSValue) = 20 (4 + 8 + 8)
> > > 
> > > Shit, the JSValue is aligned on 8 bytes!
> > > Thanks Ossy! I'll follow up with a separate patch to fix this.
> > 
> > Do you know why this is needed though on 32-bit platforms?
> 
> AFAIK the natural alignment is mandatory everywhere.
> Isn't it true on Apple platforms?

We have plenty of exceptions in our ABI.
Take https://developer.apple.com/library/prerelease/content/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARMv6FunctionCallingConventions.html for example.

Our stack also has weaker alignment.

> I think the easiest fix would be to put the JSValue member 
> to the begginning of the struct to avoid padding.

Agreed. I'll check if the order matter for perf.

[Csaba Osztrogonác]

(In reply to comment #15)
 
> I think the easiest fix would be to put the JSValue member 
> to the begginning of the struct to avoid padding.

Hmm, it didn't help. In this case the padding is added to the end, because 
in an array the first JSValue member should be aligned to 8 bytes.

[Benjamin Poulain]

(In reply to comment #18)
> (In reply to comment #15)
>  
> > I think the easiest fix would be to put the JSValue member 
> > to the begginning of the struct to avoid padding.
> 
> Hmm, it didn't help. In this case the padding is added to the end, because 
> in an array the first JSValue member should be aligned to 8 bytes.

Arg. Yep, that make sense :(