Bug 198466 – [WinCairo] Implement cpu and memory measuring functions.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Takashi Komori]

On WinCairo layout tests related to inspector doesn't work at all.
This is because lack of CPUProfilerObserver and MemoryObserver which are used in Test.js. (Source/WebInspectorUI/UserInterface/Test/Test.js)

In This ticket we aim to fix it by enabling ENABLE_RESOURCE_USAGE.

[Takashi Komori]

Created attachment 371142 [details]
Patch

[Fujii Hironori]

Comment on attachment 371142 [details]
Patch

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

> Source/WebCore/PlatformWinCairo.cmake:16
> +    page/win/ResourceUsageThreadWinCairo.cpp

Rename *Win.cpp.

> Source/WebCore/page/win/ResourceUsageThreadWinCairo.cpp:43
> +uint64_t fileTimeToUint64(FILETIME ft)

Mark these file-local functions "static".

> Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:-40
> -    return JSStringCreateWithUTF8CString("");

What about using UrlCreateFromPath?
https://docs.microsoft.com/en-us/windows/desktop/api/shlwapi/nf-shlwapi-urlcreatefrompatha

> Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:44
> +        _wsplitpath(exePath, drive, dir, nullptr, nullptr);

Why do you need to split first?

> Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:52
> +        char stubPathUTF8[MAX_PATH];

MAX_PATH shouldn't be used for UTF-8 buffer length. How about using JSStringCreateWithCharacters? Then, you don't need to convert to UTF-8.

> LayoutTests/platform/wincairo/TestExpectations:1566
>  inspector [ Skip ]

Can you unskip all inspector tests? How much failure will occur? If you can't unskip inspector tests, I think you should change the summary of this bug.

[Don Olmstead]

Comment on attachment 371142 [details]
Patch

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

> Source/WebCore/page/win/ResourceUsageOverlayWinCairo.cpp:30
> +#if ENABLE(RESOURCE_USAGE) && OS(WINDOWS)

You shouldn't need any of these OS checks.

> Source/WebCore/page/win/ResourceUsageThreadWinCairo.cpp:31
> +#if ENABLE(RESOURCE_USAGE) && OS(WINDOWS)

Ditto

[Takashi Komori]

Created attachment 371236 [details]
Patch

[Takashi Komori]

Created attachment 371237 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #2)
> Comment on attachment 371142 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371142&action=review
> 
> > Source/WebCore/PlatformWinCairo.cmake:16
> > +    page/win/ResourceUsageThreadWinCairo.cpp
> 
> Rename *Win.cpp.

Renamed.

> > Source/WebCore/page/win/ResourceUsageThreadWinCairo.cpp:43
> > +uint64_t fileTimeToUint64(FILETIME ft)
> 
> Mark these file-local functions "static".

Marked.

> > Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:-40
> > -    return JSStringCreateWithUTF8CString("");
> 
> What about using UrlCreateFromPath?
> https://docs.microsoft.com/en-us/windows/desktop/api/shlwapi/nf-shlwapi-
> urlcreatefrompatha

Changed to use UrlCreateFromPathW.
 
> > Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:44
> > +        _wsplitpath(exePath, drive, dir, nullptr, nullptr);
> 
> Why do you need to split first?

We need to determine resource directory path.
This part makes the path by splitting absolute path of WebKitTestRunner.exe.

> > Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:52
> > +        char stubPathUTF8[MAX_PATH];
> 
> MAX_PATH shouldn't be used for UTF-8 buffer length. How about using
> JSStringCreateWithCharacters? Then, you don't need to convert to UTF-8.

Changed.

> > LayoutTests/platform/wincairo/TestExpectations:1566
> >  inspector [ Skip ]
> 
> Can you unskip all inspector tests? How much failure will occur? If you
> can't unskip inspector tests, I think you should change the summary of this
> bug.

Changed.

[Takashi Komori]

(In reply to Don Olmstead from comment #3)
> Comment on attachment 371142 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371142&action=review
> 
> > Source/WebCore/page/win/ResourceUsageOverlayWinCairo.cpp:30
> > +#if ENABLE(RESOURCE_USAGE) && OS(WINDOWS)
> 
> You shouldn't need any of these OS checks.
> 
> > Source/WebCore/page/win/ResourceUsageThreadWinCairo.cpp:31
> > +#if ENABLE(RESOURCE_USAGE) && OS(WINDOWS)
> 
> Ditto

Fixed.

[Don Olmstead]

Comment on attachment 371237 [details]
Patch

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

Couple nits but LGTM overall. Fujii could you take another pass?

> Source/WebCore/page/win/ResourceUsageThreadWin.cpp:116
> +    // FIXME: Exclude the ResourceUsage thread.
> +    // FIXME: Exclude the SamplingProfiler thread.
> +    // FIXME: Classify usage per thread.
> +    data.cpuExcludingDebuggerThreads = data.cpu;

Could you open up a bug for this work and then link to it?

> Source/cmake/OptionsWinCairo.cmake:35
> +set(ENABLE_RESOURCE_USAGE ON)

This is a WebKit option so this should be moved to OptionsWin.cmake in the `if (${WTF_PLATFORM_WIN_CAIRO})` block in between WEBKIT_OPTION_BEGIN() and WEBKIT_OPTION_END()

```
WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE OFF)
```

[Don Olmstead]

(In reply to Don Olmstead from comment #8)
> Comment on attachment 371237 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371237&action=review
> > Source/cmake/OptionsWinCairo.cmake:35
> > +set(ENABLE_RESOURCE_USAGE ON)
> 
> This is a WebKit option so this should be moved to OptionsWin.cmake in the
> `if (${WTF_PLATFORM_WIN_CAIRO})` block in between WEBKIT_OPTION_BEGIN() and
> WEBKIT_OPTION_END()
> 
> ```
> WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE OFF)
> ```

Sorry ON not OFF. Copy paste error...

[Don Olmstead]

(In reply to Don Olmstead from comment #9)
> (In reply to Don Olmstead from comment #8)
> > Comment on attachment 371237 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=371237&action=review
> > > Source/cmake/OptionsWinCairo.cmake:35
> > > +set(ENABLE_RESOURCE_USAGE ON)
> > 
> > This is a WebKit option so this should be moved to OptionsWin.cmake in the
> > `if (${WTF_PLATFORM_WIN_CAIRO})` block in between WEBKIT_OPTION_BEGIN() and
> > WEBKIT_OPTION_END()
> > 
> > ```
> > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE OFF)
> > ```
> 
> Sorry ON not OFF. Copy paste error...

Actually can you add it to the experimental features list till we have the full implementation.

WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE ${ENABLE_EXPERIMENTAL_FEATURES})

[Don Olmstead]

Comment on attachment 371237 [details]
Patch

r=me

Fix the CMake part and this is good to go

[Takashi Komori]

Created attachment 371252 [details]
Patch

[Takashi Komori]

(In reply to Don Olmstead from comment #10)
> (In reply to Don Olmstead from comment #9)
> > (In reply to Don Olmstead from comment #8)
> > > Comment on attachment 371237 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=371237&action=review
> > > > Source/cmake/OptionsWinCairo.cmake:35
> > > > +set(ENABLE_RESOURCE_USAGE ON)
> > > 
> > > This is a WebKit option so this should be moved to OptionsWin.cmake in the
> > > `if (${WTF_PLATFORM_WIN_CAIRO})` block in between WEBKIT_OPTION_BEGIN() and
> > > WEBKIT_OPTION_END()
> > > 
> > > ```
> > > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE OFF)
> > > ```
> > 
> > Sorry ON not OFF. Copy paste error...
> 
> Actually can you add it to the experimental features list till we have the
> full implementation.
> 
> WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_USAGE PRIVATE
> ${ENABLE_EXPERIMENTAL_FEATURES})

FIXED.

[Takashi Komori]

(In reply to Don Olmstead from comment #8)
> Comment on attachment 371237 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371237&action=review
> 
> Couple nits but LGTM overall. Fujii could you take another pass?
> 
> > Source/WebCore/page/win/ResourceUsageThreadWin.cpp:116
> > +    // FIXME: Exclude the ResourceUsage thread.
> > +    // FIXME: Exclude the SamplingProfiler thread.
> > +    // FIXME: Classify usage per thread.
> > +    data.cpuExcludingDebuggerThreads = data.cpu;
> 
> Could you open up a bug for this work and then link to it?
> 

Opened https://bugs.webkit.org/show_bug.cgi?id=198519

[EWS Watchlist]

Comment on attachment 371252 [details]
Patch

Attachment 371252 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12370788

New failing tests:
http/wpt/service-workers/service-worker-networkprocess-crash.html

[EWS Watchlist]

Created attachment 371257 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[WebKit Commit Bot]

Comment on attachment 371252 [details]
Patch

Clearing flags on attachment: 371252

Committed r246076: <https://trac.webkit.org/changeset/246076>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51409011>