Bug 234097 - Teach the sampling profiler how to display origin data for B3 Wasm
Summary: Teach the sampling profiler how to display origin data for B3 Wasm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-09 11:42 PST by Saam Barati
Modified: 2021-12-11 16:42 PST (History)
11 users (show)

See Also:


Attachments
WIP (20.75 KB, patch)
2021-12-09 19:05 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (25.33 KB, patch)
2021-12-11 12:16 PST, Saam Barati
ysuzuki: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch (25.90 KB, patch)
2021-12-11 12:30 PST, Saam Barati
ysuzuki: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch for landing (26.03 KB, patch)
2021-12-11 12:45 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2021-12-09 11:42:13 PST
So we can start to dig into where in a function we're spending time, not just which functions.
Comment 1 Saam Barati 2021-12-09 19:05:26 PST
Created attachment 446653 [details]
WIP
Comment 2 Saam Barati 2021-12-11 12:16:11 PST
Created attachment 446895 [details]
patch
Comment 3 Yusuke Suzuki 2021-12-11 12:22:26 PST
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?
Comment 4 Saam Barati 2021-12-11 12:27:18 PST
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.
Comment 5 Yusuke Suzuki 2021-12-11 12:29:38 PST
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.
Comment 6 Saam Barati 2021-12-11 12:30:40 PST
Created attachment 446897 [details]
patch

try to fix some builds
Comment 7 Yusuke Suzuki 2021-12-11 12:39:59 PST
Comment on attachment 446897 [details]
patch

r=me if EWS gets green.
Comment 8 Saam Barati 2021-12-11 12:45:57 PST
Created attachment 446898 [details]
patch for landing

Trying to fix build errors.
Comment 9 EWS 2021-12-11 16:41:09 PST
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].
Comment 10 Radar WebKit Bug Importer 2021-12-11 16:42:18 PST
<rdar://problem/86370914>