[webkit-reviews] review granted: [Bug 115705] Implement a probe mechanism for JIT generated code : [Attachment 201009] new and improved after addressing Michael's feedback.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 8 14:21:11 PDT 2013


Michael Saboff <msaboff at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 115705: Implement a probe mechanism for JIT generated code
https://bugs.webkit.org/show_bug.cgi?id=115705

Attachment 201009: new and improved after addressing Michael's feedback.
https://bugs.webkit.org/attachment.cgi?id=201009&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=201009&action=review


r+ with comments.

> Source/JavaScriptCore/ChangeLog:16
> +	   the duration that the ProbeFunction is executing. It will be popped
of

typo at end of line *off*

> Source/JavaScriptCore/ChangeLog:25
> +	   This changeset only implements the probe mechanism for X86, and
X86_64.

Eliminate comma after X86

> Source/JavaScriptCore/jit/JITStubs.cpp:243
> +    "popl %eax" "\n"

Instead of popping and then pushing later, why don't we just read the return
pc?  Or is there a reason that we want to clear the pc off the stack?

> Source/JavaScriptCore/jit/JITStubs.cpp:554
> +    "popq %rax" "\n"

Same question as above.

> Source/WTF/wtf/Alignment.h:47
> +//	 WTF_PACK(0, 3); // yields 0.
> +//	 WTF_PACK(5, 3); // yields 8.
> +//	 WTF_PACK(8, 3); // yields 8.
> +//	 WTF_PACK(23, 3); // yields 24 i.e. 3 x 8.
> +//	 WTF_PACK(24, 3); // yields 24.
> +//
> +//	 // Pack to 16 bytes i.e. 4 bits of alignment.
> +//	 WTF_PACK(0, 4); // yields 0.
> +//	 WTF_PACK(5, 4); // yields 16.
> +//	 WTF_PACK(8, 4); // yields 16.
> +//	 WTF_PACK(23, 4); // yields 32 i.e. 2 x 16.
> +//	 WTF_PACK(24, 4); // yields 32.

Do we need all of these examples?

> Source/WTF/wtf/Alignment.h:96
> +    // Note that in binary, alignSize is always a 1 followed by N 0s.
> +    // That means maskBits is 0s followed by N 1s (as expected). We can use
> +    // this mask to mask off the low bits of a given size to produce the
> +    // nearest multiple of alignSize below that size:
> +    //
> +    //   roundDown(size) { return size & maskBits; }
> +    //
> +    // If roundDown(size) yields the aligned size below size, then
> +    // roundDown(size + alignSize) should yield the aligned size above size.

> +    // We can use this to implement our roundUp() functionality except that
> +    // it doesn't work in one case:
> +    //
> +    //   If size was already sligned to begin with, then
> +    //   roundDown(size + alignSize) will produce (size + alignSize). For
> +    //   roundUp(size), we want it to produce size instead.
> +    //
> +    // To remedy this, instead of adding alignSize, we add (alignSize - 1).
> +    // We'll call this value maxPadSize. Coincidently, this is the same
value
> +    // as maskBits. Hence,
> +    //
> +    //   If size was already aligned to begin with, then
> +    //   roundDown(size + maxPadSize) will produce size as expected.
> +    //
> +    //   If size is not aligned, then size > roundDown(size).
> +    //   Let deltaSize = size - roundDown(size). Hence,
> +    //     roundDown(size + maxPadSize)
> +    //     ==> roundDown(roundDown(size) + deltaSize + maxPadSize)
> +    //     ==> roundDown(size) + roundDown(deltaSize + maxPadSize)
> +    //
> +    //     But since maxPadSize = alignSize - 1, then
> +    //	((deltaSize + maxPadsize) > alignSize) since
> +    //	deltaSize > 1 by definition.
> +    //
> +    //     Let remainderSize = (deltaSize + maxPadsize) - alignSize.
> +    //     Note: We're guaranteed that remainderSize < alignSize.
> +    //
> +    //     Hence, (alignSize + remainderSize) == (deltaSize + maxPadsize).
> +    //     So, continuing the breakdown of roundDown(size + maxPadSize) ...
> +    //
> +    //     ==> roundDown(size) + roundDown(alignSize + remainderSize)
> +    //     ==> roundDown(size) + roundDown(alignSize) +
roundDown(remainderSize)
> +    //     ==> roundDown(size) + alignSize + 0
> +    //     ==> roundUp(size) when size if not aligned.

Seems like a long comment.  Rounding up to the next power of2 shouldn't require
near this many comments.


More information about the webkit-reviews mailing list