Bug 195012 – Make JSC memory benchmark available on OpenSource

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Don Olmstead]

It would be nice to be able to track memory usage over commits to check for regressions.

[Saam Barati]

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.

[Saam Barati]

Don, to integrate this benchmark in the playstation port, you'll need to implement the "struct MemoryFootprint" inside jsc.cpp.

[Michael Saboff]

<rdar://problem/53990898>

[Michael Saboff]

Created attachment 375637 [details]
Patch

[Dean Johnson]

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.

[Michael Saboff]

(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.

[Michael Saboff]

Created attachment 375647 [details]
Updated patch responding to review comments

[Dean Johnson]

LGTM after updates. Unofficial r+.

[Caio Lima]

(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.

[Saam Barati]

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.

[Michael Saboff]

Committed r248326: <https://trac.webkit.org/changeset/248326>

[Paulo Matos]

Thank you for this. It is really useful.