These break the Leaks tool on OS X.
Created attachment 46700 [details] The patch
Created attachment 46702 [details] The patch
Created attachment 46704 [details] The patch
Comment on attachment 46700 [details] The patch r-, i think cachedPrototypeStructure = (Structure*)1 should be static const Structure* seenFlag = (Structure*)1; cachedPrototypeStructure = seenFlag (with a better name for seenFlag)
Comment on attachment 46704 [details] The patch i'd like maciej's opinion on stripping a bit from the string hash
Comment on attachment 46700 [details] The patch bah, use a #define, otherwise r=me :(
Comment on attachment 46704 [details] The patch r=me assuming maciej is okay ith the hash size reduction
Duplicate of bug 32754/<rdar://problem/7486511>? I'm not sure if anything tracks related UntypedPtrAndBitfield issue.
Created attachment 46718 [details] New implementation for UStringImpl. Instead, merge the bits into the ref count (ty Alexey for the suggestion!) No perf degradation, so let's go with this. Restore the hash to a full 32-bits.
Comment on attachment 46704 [details] The patch This patch is no longer valid, since we're not going to steal bits from the hash. Will instead probably do something like UStringImpl, stealing bits from the refCount.
Attachment 46718 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 JavaScriptCore/runtime/UStringImpl.h:66: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] JavaScriptCore/runtime/UStringImpl.h:134: More than one command on the same line [whitespace/newline] [4] JavaScriptCore/runtime/UStringImpl.h:135: More than one command on the same line [whitespace/newline] [4] Total errors found: 3
Created attachment 46756 [details] New WebCore::StringImpl patch New patch for StringImpl, stealing bits from refCount instead of hash.
Attachment 46756 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/text/StringImpl.h:105: More than one command on the same line [whitespace/newline] [4] WebCore/platform/text/StringImpl.h:106: More than one command on the same line [whitespace/newline] [4] WebCore/platform/text/StringImpl.h:180: More than one command on the same line [whitespace/newline] [4] Total errors found: 3
Comment on attachment 46756 [details] New WebCore::StringImpl patch > + OwnPtr<const UChar*> path(fileName.copyWithNullTermination()); This needs to be OwnArrayPtr<const UChar>, not OwnPtr<const UChar*>. Because delete [] and delete without the "[]" are not the same thing. > + OwnPtr<const UChar*> m_queryWithNullTermination; Same here. > + const UChar* copyWithNullTermination() const; This should probably return PassOwnArrayPtr, not a raw pointer. Otherwise it's too easy to get the type wrong for the smart pointer (as above). Unfortunately, there is no PassOwnArrayPtr as of this writing. Another good possibility would be to have the function take in an OwnArrayPtr<const UChar>& and set it to point to the newly allocated buffer. > + static const unsigned s_refCountMask = 0xfffffffe; I personally prefer capital hex. review+ assuming you fix the OwnArrayPtr issue.
Attachment 46756 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/192656
*** Bug 32754 has been marked as a duplicate of this bug. ***
JIT changes landed in r53391.
UStringImpl changes landed in r53392.
Looks like r 53392 broke several builds.
By that, I mean it caused various SVG tests to crash on the builders: svg/custom/resource-invalidate-on-target-update.svg crashed: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r53392%20(9472)/results.html 20 tests crashed: http://build.webkit.org/results/Windows%20Debug%20(Tests)/r53392%20(8626)/results.html 3 svg tests crashed: http://build.webkit.org/results/GTK%20Linux%20Release/r53393%20(7754)/results.html svg/custom/selectSubString.html stderr svg/dom/SVGScriptElement/script-load-and-error-events.svg stderr svg/dom/SVGScriptElement/script-set-href.svg stderr The bot seems to think it was related to this checkin. I recommend we roll this out.
The crashes went away for Leopard 3 builds later and then came back again as: svg/custom/scrolling-embedded-svg-file-image-repaint-problem.html stderr http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r53397%20(9477)/results.html Looks like flakiness, very possibly related to this checkin since I've never seen this crash before.
I'm going to roll out r53392 in an effort to re-green the bots. If it doesn't help, I"m happy to roll it back in.
r53391 and r53392 rolled out by http://trac.webkit.org/changeset/53400 because of flakey crashes on Leopard, Snow Leopard, Windows, GTK, Qt bots.
WebCore::StringImpl changes landed in r53416.
WebCore changes need more work on Windows, rolling out in r53418.
Created attachment 46851 [details] WebCore::StringImpl patch take 3, re-introduce String::charactersWithNullTermination() / StringImpl::hasTerminatingNullCharacter()
Comment on attachment 46851 [details] WebCore::StringImpl patch take 3, re-introduce String::charactersWithNullTermination() / StringImpl::hasTerminatingNullCharacter() Gah! no, this will result in a string's length changing when you access the null terminated form. Nevermind, I'll go with the simple solution & burn another bit. :-(
Created attachment 46858 [details] take 4. Simple approach, add a flag back in for HasTerminatingNullCharacter.
Attachment 46858 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/text/StringImpl.h:109: More than one command on the same line [whitespace/newline] [4] WebCore/platform/text/StringImpl.h:110: More than one command on the same line [whitespace/newline] [4] WebCore/platform/text/StringImpl.h:184: More than one command on the same line [whitespace/newline] [4] Total errors found: 3 If any of these errors are false positives, please file a bug against check-webkit-style.
WebCore changes relanded in r53447.
Created attachment 46945 [details] New UStringImpl patch, without ancillary refactoring changes.
Attachment 46945 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 JavaScriptCore/runtime/UStringImpl.h:131: More than one command on the same line [whitespace/newline] [4] JavaScriptCore/runtime/UStringImpl.h:132: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 46945 [details] New UStringImpl patch, without ancillary refactoring changes. Damn, it removed my review comments! There weren’t any important ones. One thing I asked was whether we needed more ALWAYS_INLINE.
The webkit patch caused leaks - this is probably also what was causing the JSC patches as originally landed to cause tests crashes on some machines (i.e. they were causing so much leakage DRT actually ran out of memory). So, reverted in r53575. :-( *sniff*
I'm not sure which (both?) of these patches was reverted. Something tells me this probably doesn't need to be in the http://webkit.org/pending-commit list (by having patches marked r+).
Hey Eric, All patches are currently reverted, but the first one I think is probably good to go – probably didn't need reverting out in the first place, so I'll leave that r+ed & reland soon. Removed the r+ from the latter two patches, am working on updated patches for these at the mo. cheers, G.
Thanks for the quick reply.
Comment on attachment 46702 [details] The patch Cleared Oliver Hunt's review+ from obsolete attachment 46702 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 46718 [details] New implementation for UStringImpl. Cleared Oliver Hunt's review+ from obsolete attachment 46718 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 46756 [details] New WebCore::StringImpl patch Cleared Darin Adler's review+ from obsolete attachment 46756 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Created attachment 48663 [details] New StringImpl patch, also fix bufferIsInternal bug that was being triggered.
Attachment 48663 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/text/StringImpl.h:120: More than one command on the same line [whitespace/newline] [4] WebCore/platform/text/StringImpl.h:121: More than one command on the same line [whitespace/newline] [4] WebCore/platform/text/StringImpl.h:121: More than one command on the same line in if [whitespace/parens] [4] WebCore/platform/text/StringImpl.h:195: More than one command on the same line [whitespace/newline] [4] Total errors found: 4 If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 48663 [details] New StringImpl patch, also fix bufferIsInternal bug that was being triggered. > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 54731) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,38 @@ > +2010-02-12 Gavin Barraclough <barraclough@apple.com> > + > + Reviewed by NOBODY (OOPS!). > + > + https://bugs.webkit.org/show_bug.cgi?id=33731 > + Remove uses of PtrAndFlags from WebCore::StringImpl. > + > + These break the OS X Leaks tool. Move the management of null-terminated copies > + out from StringImpl to String, and use a bits stolen from the refCount to hold the LIES! Please fix the change log and add the extra assert in the StringImpl destructor. Otherwise, r=me.
(In reply to comment #43) > (From update of attachment 48663 [details]) > > Index: WebCore/ChangeLog > > =================================================================== > > --- WebCore/ChangeLog (revision 54731) > > +++ WebCore/ChangeLog (working copy) > > @@ -1,3 +1,38 @@ > > +2010-02-12 Gavin Barraclough <barraclough@apple.com> > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + https://bugs.webkit.org/show_bug.cgi?id=33731 > > + Remove uses of PtrAndFlags from WebCore::StringImpl. > > + > > + These break the OS X Leaks tool. Move the management of null-terminated copies > > + out from StringImpl to String, and use a bits stolen from the refCount to hold the > > LIES! > > Please fix the change log and add the extra assert in the StringImpl > destructor. > > Otherwise, r=me. Well, I started reviewing it, but Sam was faster than me. Thanks for doing this (and sorry for breaking leaks). Something to consider: > + static const unsigned s_refCountMask = 0xFFFFFFF0; What about static const unsigned s_refCountMask = UINT_MAX & ~(s_refCountIncrement - 1) or just static const unsigned s_refCountMask = ~(s_refCountIncrement - 1)
New webcore::stringimpl patch landed in r54736. /me crosses fingers, toes, noses.
Landed my previous UStringImpl patch in r54743. This was r+ from Darin before I marked r-, while debugging an apparent leak. The leak seems to be well understood now, it seems these patches change UStringImpl / StringImpl objects to be 32 bytes instead of 40, and this in turn triggers the pre existing bug (fixed in the StringImpl patch) that if string data falls in the subsequent heap block WebCore::StringImpl would confuse this for an internal buffer & fail to free it.
JIT patch landed in r54747.
Created attachment 48693 [details] Last patch v0.1! Needs the ChnageLog filling out.
Created attachment 48735 [details] Final Patch! Refactor StructureTransitionTable back into Structure, remove PtrAndFlags!
Attachment 48735 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 JavaScriptCore/runtime/Structure.h:249: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] JavaScriptCore/runtime/Structure.h:264: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] JavaScriptCore/runtime/Structure.h:328: More than one command on the same line [whitespace/newline] [4] JavaScriptCore/runtime/Structure.h:340: More than one command on the same line [whitespace/newline] [4] JavaScriptCore/runtime/Structure.h:341: More than one command on the same line [whitespace/newline] [4] Skipping input 'JavaScriptCore/wtf/PtrAndFlags.h': Can't open for reading Skipping input 'WebCore/ForwardingHeaders/wtf/PtrAndFlags.h': Can't open for reading Total errors found: 5 If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 48735 [details] Final Patch! Refactor StructureTransitionTable back into Structure, remove PtrAndFlags! > + , m_usingSingleSlot(true) The naming scheme for booleans where we make them finish a sentence "structure <xxx>" would mean this should be named m_isUsingSingleSlot. > + bool structureTransitionTableContains(const StructureTransitionTableHash::Key& key, JSCell* specificValue) > + Structure* structureTransitionTableGet(const StructureTransitionTableHash::Key& key, JSCell* specificValue) const > + bool structureTransitionTableHasTransition(const StructureTransitionTableHash::Key& key) const > + void structureTransitionTableRemove(const StructureTransitionTableHash::Key& key, JSCell* specificValue) > + void structureTransitionTableAdd(const StructureTransitionTableHash::Key& key, Structure* structure, JSCell* specificValue) > + TransitionTable* structureTransitionTable() const { ASSERT(!m_usingSingleSlot); return m_transitions.m_table; } > + void setStructureTransitionTable(TransitionTable* table) Seems we should name these function members transitionTableXXX since they are members of the Structure class. It also seems like the header would be a little easier to read if the function bodies were outside the class definition, marked inline. Definitions of functions that are used only inside Structure.cpp can go in that file rather than the header. r=me
Sending JavaScriptCore/ChangeLog Sending JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj Sending JavaScriptCore/bytecode/CodeBlock.h Sending JavaScriptCore/runtime/Structure.cpp Sending JavaScriptCore/runtime/Structure.h Sending JavaScriptCore/runtime/StructureTransitionTable.h Deleting JavaScriptCore/wtf/PtrAndFlags.h Sending WebCore/ChangeLog Deleting WebCore/ForwardingHeaders/wtf/PtrAndFlags.h Transmitting file data ....... Committed revision 54798. PtrAndFlags is gone, resolved fixed. A similar problem is know to exist due to our Rope implementation, but tracking that in a new bug ( https://bugs.webkit.org/show_bug.cgi?id=34964 ).
The last patch removed PtrAndFlags.h. However, references to the file still exist: JavaScriptCore/JavaScriptCore.vcproj/WTF/WTF.vcproj 444: RelativePath="..\..\wtf\PtrAndFlags.h" JavaScriptGlue/ForwardingHeaders/wtf/PtrAndFlags.h 1:#include <JavaScriptCore/PtrAndFlags.h> Any cause for worry?