We can calculate our out of line capacity by using the outOfLineSize and our knowledge about our resize policy.
Created attachment 185273 [details] Patch
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.
> 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 on attachment 185273 [details] Patch Attachment 185273 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16115313
> 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.
Created attachment 185288 [details] Patch
Comment on attachment 185288 [details] Patch Attachment 185288 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16217139
Comment on attachment 185288 [details] Patch Attachment 185288 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16201273
Comment on attachment 185288 [details] Patch Attachment 185288 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16200290
Created attachment 185294 [details] Patch
Comment on attachment 185294 [details] Patch Attachment 185294 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16218126
Created attachment 185297 [details] Patch
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.
Created attachment 185372 [details] Patch
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.
(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.
Let's use #if sizeof(size_t) == 8. This code is so low-level, that's probably more appropriate than any abstract #ifdef.
Comment on attachment 185372 [details] Patch r=me with that change
Committed r141295: <http://trac.webkit.org/changeset/141295>
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.
Reopening to address issues.
Created attachment 185651 [details] Patch
This is awesome!
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 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.
Created attachment 186415 [details] Patch
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.
Committed r141916: <http://trac.webkit.org/changeset/141916>