[JSC] Simplify the initialization of AbstractValue in the AbstractInterpreter
Created attachment 284927 [details] Patch
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
Created attachment 284928 [details] Patch
Comment on attachment 284928 [details] Patch r=me
Comment on attachment 284928 [details] Patch Clearing flags on attachment: 284928 Committed r204065: <http://trac.webkit.org/changeset/204065>
All reviewed patches have been landed. Closing bug.
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."); .
(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?
(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.
(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)
(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.
(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.
(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?
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.
(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.
(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.
(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.
(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.
(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 :(