| Summary: | Leak bots erroneously report JSC::WatchpointSet as leaking | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
| Component: | JavaScriptCore | Assignee: | Ryosuke Niwa <rniwa> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | barraclough, fpizlo, ggaren, mjs, psolanki, simon.fraser | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 106716 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Ryosuke Niwa
2013-01-23 20:48:06 PST
This appears to be due to the fact that SymbolTable does low-bit tagging when pointing at the watchpoint set. > This appears to be due to the fact that SymbolTable does low-bit tagging when pointing at the watchpoint set.
So does that mean this isn't really a leak? That the memory is actually referenced and used but leaks doesn't know about it?
*** Bug 107782 has been marked as a duplicate of this bug. *** We've found out that this is a false positive due to the fact FatEntry is a tagged pointer. I'm going to post a patch to work-around this problem in leaks by making it an "untagged" pointer. Created attachment 193423 [details]
Use 'Untagged pointer' to work around leaks' limitation
Comment on attachment 193423 [details] Use 'Untagged pointer' to work around leaks' limitation View in context: https://bugs.webkit.org/attachment.cgi?id=193423&action=review > Source/JavaScriptCore/runtime/SymbolTable.h:132 > - return m_bits & FatFlag; > + return m_bits && !(m_bits & SlimFlag); I'm somewhat concerned about the performance implication of this change. > Source/JavaScriptCore/runtime/SymbolTable.h:323 > - bitsRef = (static_cast<intptr_t>(index) << FlagBits) | NotNullFlag; > + bitsRef = (static_cast<intptr_t>(index) << FlagBits) | NotNullFlag | (bitsRef & SlimFlag); And also this dependency. It appears that the code assumed that FatFlag is never set so we could add ASSERT(!isFat()) and simply | SlimFlag here instead. Comment on attachment 193423 [details] Use 'Untagged pointer' to work around leaks' limitation View in context: https://bugs.webkit.org/attachment.cgi?id=193423&action=review Looks good, but I kind of want to know why you need to check that m_bits is non-zero in addition to checking that it doesn't have SlimFlag set. >> Source/JavaScriptCore/runtime/SymbolTable.h:132 >> + return m_bits && !(m_bits & SlimFlag); > > I'm somewhat concerned about the performance implication of this change. Why doesn't this just say !(m_bits & SlimFlag)? >> Source/JavaScriptCore/runtime/SymbolTable.h:323 >> + bitsRef = (static_cast<intptr_t>(index) << FlagBits) | NotNullFlag | (bitsRef & SlimFlag); > > And also this dependency. It appears that the code assumed that FatFlag is never set so we could add ASSERT(!isFat()) and simply | SlimFlag here instead. Looks right. *** Bug 112497 has been marked as a duplicate of this bug. *** Comment on attachment 193423 [details] Use 'Untagged pointer' to work around leaks' limitation View in context: https://bugs.webkit.org/attachment.cgi?id=193423&action=review >>> Source/JavaScriptCore/runtime/SymbolTable.h:132 >>> + return m_bits && !(m_bits & SlimFlag); >> >> I'm somewhat concerned about the performance implication of this change. > > Why doesn't this just say !(m_bits & SlimFlag)? So the reason SymbolTableEntry::isFast checks m_bits && !(m_bits & SlimFlag) is because we have emptyValueIsZero set to true in SymbolTableIndexHashTraits but we probably don’t need it here if we forced to set SlimFlag or NotNullFlag in Fast::Fast. Created attachment 193636 [details]
Simplified pack
(In reply to comment #10) > (From update of attachment 193423 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193423&action=review > > >>> Source/JavaScriptCore/runtime/SymbolTable.h:132 > >>> + return m_bits && !(m_bits & SlimFlag); > >> > >> I'm somewhat concerned about the performance implication of this change. > > > > Why doesn't this just say !(m_bits & SlimFlag)? > > So the reason SymbolTableEntry::isFast checks m_bits && !(m_bits & SlimFlag) is because we have emptyValueIsZero set to true in SymbolTableIndexHashTraits > but we probably don’t need it here if we forced to set SlimFlag or NotNullFlag in Fast::Fast. Can we just change the traits? (In reply to comment #12) > (In reply to comment #10) > > (From update of attachment 193423 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=193423&action=review > > > > >>> Source/JavaScriptCore/runtime/SymbolTable.h:132 > > >>> + return m_bits && !(m_bits & SlimFlag); > > >> > > >> I'm somewhat concerned about the performance implication of this change. > > > > > > Why doesn't this just say !(m_bits & SlimFlag)? > > > > So the reason SymbolTableEntry::isFast checks m_bits && !(m_bits & SlimFlag) is because we have emptyValueIsZero set to true in SymbolTableIndexHashTraits > > but we probably don’t need it here if we forced to set SlimFlag or NotNullFlag in Fast::Fast. > > Can we just change the traits? I'm concerned that it may negatively affect the performance but we can try. Created attachment 193707 [details]
Use emptyValueIsZero = false
Comment on attachment 193707 [details] Use emptyValueIsZero = false Clearing flags on attachment: 193707 Committed r146568: <http://trac.webkit.org/changeset/146568> All reviewed patches have been landed. Closing bug. |