Bug 108206

Summary: Structure::m_outOfLineCapacity is unnecessary
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, ggaren, kling, ojan.autocc, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Mark Hahnenberg 2013-01-29 10:59:58 PST
We can calculate our out of line capacity by using the outOfLineSize and our knowledge about our resize policy.
Comment 1 Mark Hahnenberg 2013-01-29 11:16:24 PST
Created attachment 185273 [details]
Patch
Comment 2 Geoffrey Garen 2013-01-29 11:25:37 PST
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.
Comment 3 Mark Hahnenberg 2013-01-29 11:29:41 PST
> 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
Comment 4 Build Bot 2013-01-29 11:41:28 PST
Comment on attachment 185273 [details]
Patch

Attachment 185273 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16115313
Comment 5 Geoffrey Garen 2013-01-29 11:42:29 PST
> 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.
Comment 6 Mark Hahnenberg 2013-01-29 12:41:29 PST
Created attachment 185288 [details]
Patch
Comment 7 Early Warning System Bot 2013-01-29 12:53:20 PST
Comment on attachment 185288 [details]
Patch

Attachment 185288 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16217139
Comment 8 Early Warning System Bot 2013-01-29 12:57:55 PST
Comment on attachment 185288 [details]
Patch

Attachment 185288 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16201273
Comment 9 Build Bot 2013-01-29 13:05:49 PST
Comment on attachment 185288 [details]
Patch

Attachment 185288 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16200290
Comment 10 Mark Hahnenberg 2013-01-29 13:19:23 PST
Created attachment 185294 [details]
Patch
Comment 11 Early Warning System Bot 2013-01-29 13:27:11 PST
Comment on attachment 185294 [details]
Patch

Attachment 185294 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16218126
Comment 12 Mark Hahnenberg 2013-01-29 13:30:21 PST
Created attachment 185297 [details]
Patch
Comment 13 Geoffrey Garen 2013-01-29 14:47:08 PST
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.
Comment 14 Mark Hahnenberg 2013-01-29 18:42:34 PST
Created attachment 185372 [details]
Patch
Comment 15 Sam Weinig 2013-01-29 22:36:38 PST
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.
Comment 16 Mark Hahnenberg 2013-01-30 08:22:15 PST
(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.
Comment 17 Geoffrey Garen 2013-01-30 11:30:54 PST
Let's use #if sizeof(size_t) == 8. This code is so low-level, that's probably more appropriate than any abstract #ifdef.
Comment 18 Geoffrey Garen 2013-01-30 11:31:04 PST
Comment on attachment 185372 [details]
Patch

r=me with that change
Comment 19 Mark Hahnenberg 2013-01-30 12:06:44 PST
Committed r141295: <http://trac.webkit.org/changeset/141295>
Comment 20 Darin Adler 2013-01-30 17:45:08 PST
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.
Comment 21 Mark Hahnenberg 2013-01-30 18:29:40 PST
Reopening to address issues.
Comment 22 Mark Hahnenberg 2013-01-30 18:33:10 PST
Created attachment 185651 [details]
Patch
Comment 23 Andreas Kling 2013-01-31 05:34:22 PST
This is awesome!
Comment 24 Geoffrey Garen 2013-01-31 14:11:42 PST
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.
Comment 25 Darin Adler 2013-01-31 19:11:44 PST
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.
Comment 26 Mark Hahnenberg 2013-02-04 10:28:53 PST
Created attachment 186415 [details]
Patch
Comment 27 Darin Adler 2013-02-04 11:42:41 PST
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.
Comment 28 Mark Hahnenberg 2013-02-05 12:45:01 PST
Committed r141916: <http://trac.webkit.org/changeset/141916>