Bug 234097 – Teach the sampling profiler how to display origin data for B3 Wasm

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

So we can start to dig into where in a function we're spending time, not just which functions.

[Saam Barati]

Created attachment 446653 [details]
WIP

[Saam Barati]

Created attachment 446895 [details]
patch

[Yusuke Suzuki]

Comment on attachment 446895 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446895&action=review

> Source/JavaScriptCore/wasm/WasmCalleeRegistry.h:88
> +    void addPCToCodeOriginMap(Callee* callee, Box<PCToCodeOriginMap> originMap)
> +    {
> +        Locker locker { m_lock };
> +        ASSERT(isValidCallee(callee));
> +        auto addResult = m_pcToCodeOriginMaps.add(callee, WTFMove(originMap));
> +        RELEASE_ASSERT(addResult.isNewEntry);
> +    }
> +
> +    Box<PCToCodeOriginMap> codeOriginMap(Callee* callee)  WTF_REQUIRES_LOCK(m_lock)
> +    {
> +        ASSERT(isValidCallee(callee));
> +        auto iter = m_pcToCodeOriginMaps.find(callee);
> +        if (iter != m_pcToCodeOriginMaps.end())
> +            return iter->value;
> +        return nullptr;
> +    }

Why don't we put m_codeOriginMap to the Callee instead of holding it in this hashmap?

[Saam Barati]

Comment on attachment 446895 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446895&action=review

>> Source/JavaScriptCore/wasm/WasmCalleeRegistry.h:88
>> +    }
> 
> Why don't we put m_codeOriginMap to the Callee instead of holding it in this hashmap?

I didn't want to grow Callee by one pointer for a field that's non null only for debugging.

[Yusuke Suzuki]

Comment on attachment 446895 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446895&action=review

r=me if EWS gets green.

>>> Source/JavaScriptCore/wasm/WasmCalleeRegistry.h:88
>>> +    }
>> 
>> Why don't we put m_codeOriginMap to the Callee instead of holding it in this hashmap?
> 
> I didn't want to grow Callee by one pointer for a field that's non null only for debugging.

I think, if it is once enabled, it consumes significant amount of memory instead (HashTable is super large, it typically allocates 2x-4x more memory to maintain O(1) access. And we need to hold pair of Callee*, Box<>, which is 2x more than just having a pointer to Box<>), but so long as it is enabled in a certain case, I'm OK.

[Saam Barati]

Created attachment 446897 [details]
patch

try to fix some builds

[Yusuke Suzuki]

Comment on attachment 446897 [details]
patch

r=me if EWS gets green.

[Saam Barati]

Created attachment 446898 [details]
patch for landing

Trying to fix build errors.

[EWS]

Committed r286920 (245146@main): <https://commits.webkit.org/245146@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446898 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86370914>