Bug 108206 – Structure::m_outOfLineCapacity is unnecessary

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

We can calculate our out of line capacity by using the outOfLineSize and our knowledge about our resize policy.

[Mark Hahnenberg]

Created attachment 185273 [details]
Patch

[Geoffrey Garen]

Comment on attachment 185273 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/Structure.h:187
> +            unsigned sizeOutOfLine = outOfLineSize();

It's a pet peeve of mine when we invent two words for the same thing just to avoid compiler problems.

I like to work around that issue by using "this->" instead of inventing a new name:

     unsigned outOfLineSize = this->outOfLineSize();

I'd suggest that technique here.

> Source/JavaScriptCore/runtime/Structure.h:196
> +            return round(pow(outOfLineGrowthFactor, ceil(log(sizeOutOfLine) / log(outOfLineGrowthFactor))));

This math looks expensive. Are we sure it's not hot? Since we're working with powers of two, there are probably good bit shifty ways to save here.

[Mark Hahnenberg]

> I'd suggest that technique here.

Okiedokie.

> > Source/JavaScriptCore/runtime/Structure.h:196
> > +            return round(pow(outOfLineGrowthFactor, ceil(log(sizeOutOfLine) / log(outOfLineGrowthFactor))));
> 
> This math looks expensive. Are we sure it's not hot? Since we're working with powers of two, there are probably good bit shifty ways to save here.

I thought about that, but there was nothing immediately available that I could see. WTF has roundUpToMultipleOf, but we need nextHighestPowerOfTwo or something. There's this guy here: http://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2 , which I could add to StdLibExtras.h

[Build Bot]

Comment on attachment 185273 [details]
Patch

Attachment 185273 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16115313

[Geoffrey Garen]

> WTF has roundUpToMultipleOf, but we need nextHighestPowerOfTwo or something. There's this guy here: http://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2 , which I could add to StdLibExtras.h

Sure, let's do that, as template<size_t exponent> roundUpToPowerOf.

[Mark Hahnenberg]

Created attachment 185288 [details]
Patch

[Early Warning System Bot]

Comment on attachment 185288 [details]
Patch

Attachment 185288 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16217139

[Early Warning System Bot]

Comment on attachment 185288 [details]
Patch

Attachment 185288 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16201273

[Build Bot]

Comment on attachment 185288 [details]
Patch

Attachment 185288 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16200290

[Mark Hahnenberg]

Created attachment 185294 [details]
Patch

[Early Warning System Bot]

Comment on attachment 185294 [details]
Patch

Attachment 185294 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16218126

[Mark Hahnenberg]

Created attachment 185297 [details]
Patch

[Geoffrey Garen]

Comment on attachment 185297 [details]
Patch

Let's add a compile assert vs runtime assert version here, and remove the slow path since it's unused, like we do for roundUpToMultipleOf.

[Mark Hahnenberg]

Created attachment 185372 [details]
Patch

[Sam Weinig]

Comment on attachment 185372 [details]
Patch

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

> Source/WTF/wtf/MathExtras.h:451
> +#if USE(JSVALUE64)
> +    v |= v >> 32;
> +#endif

This doesn't seem like the right #ifdef for such a common header.

[Mark Hahnenberg]

(In reply to comment #15)
> (From update of attachment 185372 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185372&action=review
> 
> > Source/WTF/wtf/MathExtras.h:451
> > +#if USE(JSVALUE64)
> > +    v |= v >> 32;
> > +#endif
> 
> This doesn't seem like the right #ifdef for such a common header.

Is there another way to say "any 64-bit system"? I looked through Platform.h and it didn't seem like there was.

[Geoffrey Garen]

Let's use #if sizeof(size_t) == 8. This code is so low-level, that's probably more appropriate than any abstract #ifdef.

[Geoffrey Garen]

Comment on attachment 185372 [details]
Patch

r=me with that change

[Mark Hahnenberg]

Committed r141295: <http://trac.webkit.org/changeset/141295>

[Darin Adler]

Comment on attachment 185372 [details]
Patch

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

>>> Source/WTF/wtf/MathExtras.h:451
>>> +#endif
>> 
>> This doesn't seem like the right #ifdef for such a common header.
> 
> Is there another way to say "any 64-bit system"? I looked through Platform.h and it didn't seem like there was.

#if defined(__LP64__) && __LP64__ isn't right either.

I think the right way to do this is to overload this function for both uint32_t and uint64_t rather than defining it for size_t.

[Mark Hahnenberg]

Reopening to address issues.

[Mark Hahnenberg]

Created attachment 185651 [details]
Patch

[Andreas Kling]

This is awesome!

[Geoffrey Garen]

Comment on attachment 185651 [details]
Patch

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

r=me

> Source/WTF/wtf/MathExtras.h:434
> +#define BITWISE_LOG2_32(v) \

This should just be an inline function.

[Darin Adler]

Comment on attachment 185651 [details]
Patch

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

I’m not sure of the value of implementing all these unused specializations of the template function. Currently we are using only the uint32_t version of roundUpToThePowerOf<2>, and I am not sure it’s good to have a super-slow implementation that is more general yet likely to never be useful anywhere. Seems unlikely we would actually want to use the version that converts to double in practice. I’d rather see a compile time error. That “be careful” comment is a sure giveaway we might not want this.

>> Source/WTF/wtf/MathExtras.h:434
>> +#define BITWISE_LOG2_32(v) \
> 
> This should just be an inline function.

It would have to be an inline function template, not just a function, to accommodate both integer types. I’m not sure you really want that.

> Source/WTF/wtf/MathExtras.h:442
> +template<uint64_t exponent> inline uint64_t roundUpToPowerOf(uint64_t v)

Why does the exponent need to be the same type as the number we are rounding? That’s a little strange.

[Mark Hahnenberg]

Created attachment 186415 [details]
Patch

[Darin Adler]

Comment on attachment 186415 [details]
Patch

OK. But you could still have uses a template, and just not implement it for all those other powers and types at this time.

[Mark Hahnenberg]

Committed r141916: <http://trac.webkit.org/changeset/141916>