[Webkit-unassigned] [Bug 214142] [WTF] Fix PackedAlignedPtr for X86_64 canonical addresses

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=214142

--- Comment #21 from Mark Lam <mark.lam at apple.com> ---
(In reply to Jim Mason from comment #17)
> (In reply to Mark Lam from comment #16)
> > (In reply to Jim Mason from comment #15)
> > > (In reply to Yusuke Suzuki from comment #14)
> > > > Comment on attachment 403897 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=403897&action=review
> > > > 
> > > > > Source/WTF/wtf/Packed.h:147
> > > > > +#if CPU(X86_64)
> > > > 
> > > > When doing that, please do it only in Solaris.
> > > 
> > > I was planning to make the changes requested by Mark tomorrow, but the whole
> > > point of this patch is to provide support for X86_64 canonical addressing in
> > > webkit.  It is not just a Solaris thing.
> > > 
> > > If webkit does not want to do the right thing for X86_64, let me know and I
> > > will  just close the bug.
> > 
> > Just to clarify.  I think Yusuke's point is not that this is strictly
> > incorrect, but that it is unnecessary for any other OS than Solaris.  In
> > WebKit, we do a lot of work to make sure the code is optimal.  While this
> > extra work does not hurt in terms of correctness, it is guaranteed to be an
> > effective no-op on any OS that is not Solaris because for other OSes, user
> > space never has an address with a set bit 47.  So, it is not incorrect to
> > omit this extra code for any OS that is not Solaris.  Yusuke's concern here
> > is with regards to performance.
> 
> With -O2 or -O3 in gcc, the test becomes two instructions, a bit test and a
> jump.  Probably similar in other compilers.  In the PackedAlignedPtr getter,
> there is already a test -- if (isAlignmentShiftProfitable) -- that is
> probably a no op on most every platform.  This test is similarly trivial.

isAlignmentShiftProfitable is a constexpr.  This check will be constant folded.  The shift will either be emitted unconditionally, or elided away.  Hence, this is not a justification for adding a conditional branch into this code, especially one that we know are not needed for OS(DARWIN), OS(LINUX) and OS(WIN).

Also, this get() accessor is used all over the place.  Yusuke is the engineer who applied PackedAlignedPtr pointers throughout the code, and he did a lot of benchmarking back then to ensure that there is no performance regression when it was applied.  I trust he has good reason to ask to have this #if'd out.  This performance issue is also why I asked what OS you were seeing this on in the first place, and I tried to take the time to think about whether we really need this.  I would take Yusuke's word on this over mine.

> The reason I submit this patch is that an operating system can map memory
> however it likes.  If webkit does the right thing, it ensures every OS will
> work correctly, no matter what.  As it stands now, the operating system has
> to make 'the right choices' based on the current assumptions, or webkit will
> fail, which means someone has to chase bugs down.

Your claim here is not correct.  We're not assuming that the OS will make this choice.  We know for a fact that OS(DARWIN), OS(LINUX)  (see https://www.kernel.org/doc/html/latest/x86/x86_64/mm.html?highlight=virtual%20memory), and OS(WIN) (see https://docs.microsoft.com/en-us/windows/win32/memory/memory-limits-for-windows-releases#memory-and-address-space-limits) do not need this.  It's in their ABI.  And the software is written based on that ABI.  Solaris is the weird duck here.

> As you pointed out, the current TEST(WTF_Packed, AssignAndGet) is testing an
> address that is *invalid* for the X86_64 processor architecture.  And in the
> web assembly code, a signed value is being used for a pointer test. 
> Cleaning up these sorts of things helps make webkit robust.

Yes, there is value in this patch.  We're not rejecting it.  We're just trying to refine it.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200710/f6182cab/attachment.htm>