Bug 195012 - Make JSC memory benchmark available on OpenSource
Summary: Make JSC memory benchmark available on OpenSource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-25 12:11 PST by Don Olmstead
Modified: 2019-09-03 03:56 PDT (History)
12 users (show)

See Also:


Attachments
Patch (38.48 KB, patch)
2019-08-06 11:53 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Updated patch responding to review comments (38.38 KB, patch)
2019-08-06 13:23 PDT, Michael Saboff
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2019-02-25 12:11:20 PST
It would be nice to be able to track memory usage over commits to check for regressions.
Comment 1 Saam Barati 2019-02-25 15:11:29 PST
Spoke with Michael and Yusuke today. They agree we should be able to open source our current memory benchmark that builds off of JetStream 2. Assigning to Michael since he said he's willing to make that change.
Comment 2 Saam Barati 2019-02-25 15:12:23 PST
Don, to integrate this benchmark in the playstation port, you'll need to implement the "struct MemoryFootprint" inside jsc.cpp.
Comment 3 Michael Saboff 2019-08-06 10:10:22 PDT
<rdar://problem/53990898>
Comment 4 Michael Saboff 2019-08-06 11:53:11 PDT
Created attachment 375637 [details]
Patch
Comment 5 Dean Johnson 2019-08-06 12:14:43 PDT
Comment on attachment 375637 [details]
Patch

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

Looks good overall; few minor nits on styling. Unofficial r+.

> PerformanceTests/JetStream2/RAMification.py:56
> +    sum = 0.0

Nit: The code below can be written a bit more succinctly:
return sum(values) / len(values)

> PerformanceTests/JetStream2/RAMification.py:153
> +        self.environmentVars[variable] = value

Would it be better to use os.environ here? You can set environment variables like so:
os.environ[variable] = value

> PerformanceTests/JetStream2/RAMification.py:156
> +        self.environmentVars.pop(variable, None)

Ditto.

> PerformanceTests/JetStream2/RAMification.py:193
> +            args.extend(extraOptions)

Nit: Might be good to add a newline here to help separate the distinct if blocks.

> PerformanceTests/JetStream2/RAMification.py:218
> +    main.hasFailedRuns = False

Might be better to use a different variable name for this since main() is the function this is within.

> PerformanceTests/JetStream2/RAMification.py:245
> +                testName = test[0]

Nit: Might be more clear to write this like so:

for testInfo in testList:
    if isinstance(test, tuple):
        testName, test, weight = testInfo
    else:
        testName, test, weight = testInfo, testInfo, 1

> PerformanceTests/JetStream2/RAMification.py:261
> +                    for count in range(0, weight):

Nit: The "0" passed to range() is unnecessary (unless you're going for explicitness, which would also be fine).

> PerformanceTests/JetStream2/RAMification.py:265
> +                    for count in range(0, weight):

Ditto.
Comment 6 Michael Saboff 2019-08-06 13:22:36 PDT
(In reply to Dean Johnson from comment #5)
> Comment on attachment 375637 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375637&action=review
> 
> Looks good overall; few minor nits on styling. Unofficial r+.
> 
> > PerformanceTests/JetStream2/RAMification.py:56
> > +    sum = 0.0

Fixed

> Nit: The code below can be written a bit more succinctly:
> return sum(values) / len(values)
> 
> > PerformanceTests/JetStream2/RAMification.py:153
> > +        self.environmentVars[variable] = value
> 
> Would it be better to use os.environ here? You can set environment variables
> like so:
> os.environ[variable] = value
> 
> > PerformanceTests/JetStream2/RAMification.py:156
> > +        self.environmentVars.pop(variable, None)
> 
> Ditto.

These environment variables are for the child process.  They are used differently for the LocalRunner and the device runner.  In both cases, the list of environment variables is used in runOneTest to setup the environment of each test before it starts.  It is quite possible that using the os.environ list to set up PATH or something similar environment variable would cause this script to fail to launch a subprocess.

> > PerformanceTests/JetStream2/RAMification.py:193
> > +            args.extend(extraOptions)
> 
> Nit: Might be good to add a newline here to help separate the distinct if
> blocks.

Fixed.

> > PerformanceTests/JetStream2/RAMification.py:218
> > +    main.hasFailedRuns = False

Changed the name to simply hasFailedRuns.

> Might be better to use a different variable name for this since main() is
> the function this is within.
> 
> > PerformanceTests/JetStream2/RAMification.py:245
> > +                testName = test[0]
> 
> Nit: Might be more clear to write this like so:
> 
> for testInfo in testList:
>     if isinstance(test, tuple):
>         testName, test, weight = testInfo
>     else:
>         testName, test, weight = testInfo, testInfo, 1

Made these changes.

> > PerformanceTests/JetStream2/RAMification.py:261
> > +                    for count in range(0, weight):
> 
> Nit: The "0" passed to range() is unnecessary (unless you're going for
> explicitness, which would also be fine).
> 
> > PerformanceTests/JetStream2/RAMification.py:265
> > +                    for count in range(0, weight):
> 
> Ditto.

I like the clarity of the 0 for the start of the range.
Comment 7 Michael Saboff 2019-08-06 13:23:07 PDT
Created attachment 375647 [details]
Updated patch responding to review comments
Comment 8 Dean Johnson 2019-08-06 13:46:26 PDT
LGTM after updates. Unofficial r+.
Comment 9 Caio Lima 2019-08-06 14:31:26 PDT
(In reply to Saam Barati from comment #2)
> Don, to integrate this benchmark in the playstation port, you'll need to
> implement the "struct MemoryFootprint" inside jsc.cpp.

Just for the record, there is a bug to implement this on Linux-based ports here: https://bugs.webkit.org/show_bug.cgi?id=200391.
Comment 10 Saam Barati 2019-08-06 15:10:36 PDT
Comment on attachment 375647 [details]
Updated patch responding to review comments

r=me

Can we also make it so we stub out the system headers so open source builds can actually measure memory? It seems silly not to do this at this point.
Comment 11 Michael Saboff 2019-08-06 16:24:28 PDT
Committed r248326: <https://trac.webkit.org/changeset/248326>
Comment 12 Paulo Matos 2019-09-03 03:56:24 PDT
Thank you for this. It is really useful.