[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