Bug 155561 - Replace all of the various non-working and non-compiling sampling profiler hacks with a single super hack
Summary: Replace all of the various non-working and non-compiling sampling profiler ha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-16 14:59 PDT by Filip Pizlo
Modified: 2016-03-18 04:41 PDT (History)
12 users (show)

See Also:


Attachments
work in progress (110.70 KB, patch)
2016-03-16 14:59 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
maybe the patch (126.67 KB, patch)
2016-03-16 17:56 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
better patch (126.85 KB, patch)
2016-03-17 11:56 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch? (127.83 KB, patch)
2016-03-17 12:28 PDT, Filip Pizlo
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-03-16 14:59:04 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2016-03-16 14:59:34 PDT
Created attachment 274223 [details]
work in progress
Comment 2 Filip Pizlo 2016-03-16 17:56:21 PDT
Created attachment 274247 [details]
maybe the patch
Comment 3 WebKit Commit Bot 2016-03-16 17:58:48 PDT
Attachment 274247 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/SuperSampler.h:33:  g_superSamplerCount is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/SuperSampler.cpp:36:  g_superSamplerCount is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/SuperSampler.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Filip Pizlo 2016-03-17 11:56:18 PDT
Created attachment 274307 [details]
better patch
Comment 5 WebKit Commit Bot 2016-03-17 12:00:40 PDT
Attachment 274307 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/SuperSampler.h:33:  g_superSamplerCount is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/SuperSampler.cpp:36:  g_superSamplerCount is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/SuperSampler.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Filip Pizlo 2016-03-17 12:28:36 PDT
Created attachment 274314 [details]
the patch?
Comment 7 WebKit Commit Bot 2016-03-17 12:30:16 PDT
Attachment 274314 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/SuperSampler.h:33:  g_superSamplerCount is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/SuperSampler.cpp:36:  g_superSamplerCount is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/SuperSampler.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Saam Barati 2016-03-17 13:43:23 PDT
Comment on attachment 274314 [details]
the patch?

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6482
> +        bool sample = false;
> +
> +        if (sample)
> +            m_out.incrementSuperSamplerCount();
> +        

I'm not a big fan of checking this in. I think it's better to let individual users of the profiler do it.
I think this just clutters the code.
Comment 9 Filip Pizlo 2016-03-17 17:54:41 PDT
Landed in http://trac.webkit.org/changeset/198364
Comment 10 Csaba Osztrogonác 2016-03-18 00:21:56 PDT
(In reply to comment #9)
> Landed in http://trac.webkit.org/changeset/198364

It broke the cloop build: (you could have noticed it on build.webkit.org ...)

/Volumes/Data/slave/elcapitan-cloop-debug/build/Source/JavaScriptCore/bytecode/SuperSampler.cpp:61:21: error: use of undeclared identifier 'dataLog'
                    dataLog("WARNING: Super sampler undercount detected!\n");
                    ^
/Volumes/Data/slave/elcapitan-cloop-debug/build/Source/JavaScriptCore/bytecode/SuperSampler.cpp:74:5: error: use of undeclared identifier 'dataLog'
    dataLog("Percent time behind super sampler flag: ", percentage, "\n");
    ^
2 errors generated.
Comment 11 Csaba Osztrogonác 2016-03-18 00:23:09 PDT
(In reply to comment #9)
> Landed in http://trac.webkit.org/changeset/198364

just to document, GTK buildfix landed in https://trac.webkit.org/changeset/198384
Comment 12 Csaba Osztrogonác 2016-03-18 04:41:34 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > Landed in http://trac.webkit.org/changeset/198364
> 
> It broke the cloop build: (you could have noticed it on build.webkit.org ...)
> 
> /Volumes/Data/slave/elcapitan-cloop-debug/build/Source/JavaScriptCore/
> bytecode/SuperSampler.cpp:61:21: error: use of undeclared identifier
> 'dataLog'
>                     dataLog("WARNING: Super sampler undercount detected!\n");
>                     ^
> /Volumes/Data/slave/elcapitan-cloop-debug/build/Source/JavaScriptCore/
> bytecode/SuperSampler.cpp:74:5: error: use of undeclared identifier 'dataLog'
>     dataLog("Percent time behind super sampler flag: ", percentage, "\n");
>     ^
> 2 errors generated.

Fix landed in https://trac.webkit.org/changeset/198396.
Please check build.webkit.org after landing patches next time.