Bug 174478

Created attachment 315383 [details] Patch

Comment on attachment 315383 [details] Patch FYI, memoryCost() needs to be thread-safe because it can be called from a concurrent GC. I see you're doing some iteration of collections without a lock. Is this safe?

(In reply to Mark Lam from comment #2) > Comment on attachment 315383 [details] > Patch > > FYI, memoryCost() needs to be thread-safe. This is not a JSC / WebCore memoryCost. This is an estimated size in bytes being added to our JSON value class (InspectorValue). It doesn't have the same contracts as the other memoryCost methods involved in GC.

Comment on attachment 315383 [details] Patch Maybe we can add a WebKitAPI Test for this. Did you test this with a bunch of cases or did you just test this with your full canvas inspection?

(In reply to Joseph Pecoraro from comment #3) > (In reply to Mark Lam from comment #2) > > Comment on attachment 315383 [details] > > Patch > > > > FYI, memoryCost() needs to be thread-safe. > > This is not a JSC / WebCore memoryCost. This is an estimated size in bytes > being added to our JSON value class (InspectorValue). It doesn't have the > same contracts as the other memoryCost methods involved in GC. OK, good to know. Thanks.

Created attachment 315766 [details] Patch

Comment on attachment 315766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315766&action=review r=me > Source/JavaScriptCore/inspector/InspectorValues.h:111 > + virtual size_t memoryCost() const; I feel like somewhere in this header should be a comment that this class expects non-cyclic values, as we cannot serialize cyclic structures to JSON. Right now we would infinite loop / hang attempting to either toJSONString() or memoryCost() an InspectorValue with cyclic references. At this point I think we should include Apple in the copyright of this header / implementation. We've made enough changes over the years and this is a new API on the class. > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:2719 > FE217ECB1640A54A0052988B /* JavaScriptCore */ = { This group already existed? How peculiar! > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:3275 > + 95646E5B1F1DB60E00DE0EB9 /* InspectorValue.cpp in Sources */, I suggest running: $ sort-Xcode-project-file TestWebKitAPI.xcodeproj/project.pbxproj $ git add -p TestWebKitAPI.xcodeproj/project.pbxproj # Either add the entire file, or only the lines with this new InspectorValue file so it is sorted appropriately. > Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp:35 > +TEST(InspectorValue, MemoryCost_Null) I think its more common to have names like "MemoryCostNull" than "MemoryCost_Null", but I'll leave that up to you. > Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp:40 > + EXPECT_LT(0U, memoryCost); > + EXPECT_GE(8U, memoryCost); Gosh this is hard to read... It would read so much better if it was flipped. For example: EXPECT_LE(memoryCost, 8U); EXPECT_GT(memoryCost, 0U); It looks like other tests use this kind of formatting as well. So lets switch. > Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp:97 > + EXPECT_LT(valueA->memoryCost(), valueB->memoryCost()); Notice here it reads naturally. A is less than B, B is less than C, ... > Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp:106 > + Ref<InspectorValue> valueA = InspectorValue::create("t"); > + Ref<InspectorValue> valueB = InspectorValue::create("ð"); > + EXPECT_LE(valueA->memoryCost(), valueB->memoryCost()); Are the actual values here equal or different? (Keep the test the same, I just want to verify that non-8 bit strings are larger. > Tools/TestWebKitAPI/Tests/JavaScriptCore/InspectorValue.cpp:189 > + Ref<InspectorArray> valueA = InspectorArray::create(); > + valueA->pushValue(InspectorValue::null()); > + > + Ref<InspectorArray> valueB = InspectorArray::create(); > + valueB->pushValue(InspectorValue::null()); > + valueB->pushValue(InspectorValue::null()); > + > + Ref<InspectorArray> valueC = InspectorArray::create(); > + valueC->pushValue(InspectorValue::null()); > + valueC->pushValue(InspectorValue::null()); > + valueC->pushValue(InspectorValue::null()); > + > + Ref<InspectorArray> valueD = InspectorArray::create(); > + valueD->pushValue(InspectorValue::null()); > + valueD->pushValue(InspectorValue::null()); > + valueD->pushValue(InspectorValue::null()); > + valueD->pushValue(InspectorValue::null()); You could simplify this test quite a bite by using a single array and grabbing its memory size as you grow it: Ref<InspectorArray> value = InspectorArray::create(); size_t memoryCost0 = value->memoryCost(); value->pushValue(InspectorValue::null()); size_t memoryCost1 = value->memoryCost(); value->pushValue(InspectorValue::null()); size_t memoryCost2 = value->memoryCost(); value->pushValue(InspectorValue::null()); size_t memoryCost3 = value->memoryCost(); EXPECT_LT(memoryCost0, memoryCost1); EXPECT_LT(memoryCost1, memoryCost2); EXPECT_LT(memoryCost2, memoryCost3); You can do the same for the object growing test.

Created attachment 315809 [details] Patch

Comment on attachment 315809 [details] Patch Clearing flags on attachment: 315809 Committed r219624: <http://trac.webkit.org/changeset/219624>

All reviewed patches have been landed. Closing bug.