Bug 174478 – Web Inspector: Add memoryCost to Inspector Protocol objects

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

This can be used to apply limits for protocol messages, possibly even for chunking.

[Devin Rousso]

Created attachment 315383 [details]
Patch

[Mark Lam]

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?

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

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?

[Mark Lam]

(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.

[Devin Rousso]

Created attachment 315766 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 315809 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 315809 [details]
Patch

Clearing flags on attachment: 315809

Committed r219624: <http://trac.webkit.org/changeset/219624>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.