Bug 185932 – Make a memory test where we can validate JSCs mini memory mode

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 341161 [details]
WIP

[Saam Barati]

Created attachment 341236 [details]
patch

[EWS Watchlist]

Attachment 341236 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/testmem/testmem.mm:34:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Source/JavaScriptCore/testmem/testmem.mm:35:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 341161 [details]
WIP

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

Looks good, just some nits on the style while you iterate.

> Source/JavaScriptCore/testmem/testmem.mm:33
> +    fprintf(stderr,

Why not stdout?

> Source/JavaScriptCore/testmem/testmem.mm:61
> +
> +

Style: Some newline wonkiness. I'd reduce the double newline to one. And include a single newline between the evaluation work with the contexts and the output at the end.

> Source/JavaScriptCore/testmem/testmem.mm:71
> +    fprintf(stdout, "time: %lf\n", time);

printf instead of fprintf(stdout)?

> Tools/Scripts/run-testmem:43
> +    if !output.length
> +        $stderr.puts "Error: must specify --jsc <path>"
> +        exit 1
> +    end

Is this error message correct? It doesn't look like this script accepts --jsc. Maybe:

    "Error: Could not find build directory"

> Tools/Scripts/run-testmem:50
> +    return Pathname.new(getBuildDirectory()).join("testmem").to_s

Do you want sanity check that testmem exists?

    if !File.exists?(testmemPath)
        $stderr.puts "Error: No testmem binary found in <build>/Release"
        exit 1
    end

> Tools/Scripts/run-testmem:69
> +    mempath = getTestmemPath
> +    buildDir = getBuildDirectory

Seems a shame this runs Tools/Scripts/webkit-build-directory twice.
How about getting the build directory once, and passing it into getTestmemPath(buildDir)?

> Tools/Scripts/run-testmem:71
> +    (0..3).to_a.each {

Style: Just `(0..3).each { ... }` should work without the `to_a`.

[Saam Barati]

(In reply to Joseph Pecoraro from comment #4)
> Comment on attachment 341161 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341161&action=review
> 
> Looks good, just some nits on the style while you iterate.
> 
> > Source/JavaScriptCore/testmem/testmem.mm:33
> > +    fprintf(stderr,
> 
> Why not stdout?
> 
> > Source/JavaScriptCore/testmem/testmem.mm:61
> > +
> > +
> 
> Style: Some newline wonkiness. I'd reduce the double newline to one. And
> include a single newline between the evaluation work with the contexts and
> the output at the end.
> 
> > Source/JavaScriptCore/testmem/testmem.mm:71
> > +    fprintf(stdout, "time: %lf\n", time);
> 
> printf instead of fprintf(stdout)?
> 
> > Tools/Scripts/run-testmem:43
> > +    if !output.length
> > +        $stderr.puts "Error: must specify --jsc <path>"
> > +        exit 1
> > +    end
> 
> Is this error message correct? It doesn't look like this script accepts
> --jsc. Maybe:
> 
>     "Error: Could not find build directory"
> 
> > Tools/Scripts/run-testmem:50
> > +    return Pathname.new(getBuildDirectory()).join("testmem").to_s
> 
> Do you want sanity check that testmem exists?
> 
>     if !File.exists?(testmemPath)
>         $stderr.puts "Error: No testmem binary found in <build>/Release"
>         exit 1
>     end
> 
> > Tools/Scripts/run-testmem:69
> > +    mempath = getTestmemPath
> > +    buildDir = getBuildDirectory
> 
> Seems a shame this runs Tools/Scripts/webkit-build-directory twice.
> How about getting the build directory once, and passing it into
> getTestmemPath(buildDir)?
> 
> > Tools/Scripts/run-testmem:71
> > +    (0..3).to_a.each {
> 
> Style: Just `(0..3).each { ... }` should work without the `to_a`.

Sorry I meant to obsolete the WIP. You should look at the patch

[Saam Barati]

Comment on attachment 341161 [details]
WIP

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

>> Source/JavaScriptCore/testmem/testmem.mm:71
>> +    fprintf(stdout, "time: %lf\n", time);
> 
> printf instead of fprintf(stdout)?

I can switch to printf

>> Tools/Scripts/run-testmem:43
>> +    end
> 
> Is this error message correct? It doesn't look like this script accepts --jsc. Maybe:
> 
>     "Error: Could not find build directory"

Yeah this is copy paste error. Will fix.

>> Tools/Scripts/run-testmem:50
>> +    return Pathname.new(getBuildDirectory()).join("testmem").to_s
> 
> Do you want sanity check that testmem exists?
> 
>     if !File.exists?(testmemPath)
>         $stderr.puts "Error: No testmem binary found in <build>/Release"
>         exit 1
>     end

Sure

>> Tools/Scripts/run-testmem:69
>> +    buildDir = getBuildDirectory
> 
> Seems a shame this runs Tools/Scripts/webkit-build-directory twice.
> How about getting the build directory once, and passing it into getTestmemPath(buildDir)?

This is fixed in the patch up for review

>> Tools/Scripts/run-testmem:71
>> +    (0..3).to_a.each {
> 
> Style: Just `(0..3).each { ... }` should work without the `to_a`.

Will fix

[Caio Lima]

Comment on attachment 341236 [details]
patch

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

> PerformanceTests/testmem/air.js:1


I think it wasn't supposed to be here.

[Saam Barati]

Created attachment 341241 [details]
patch

[Saam Barati]

Created attachment 341242 [details]
patch

[EWS Watchlist]

Attachment 341242 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/testmem/testmem.mm:34:  Extra space between uint8_t and ri_uuid  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/testmem/testmem.mm:34:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 341245 [details]
patch

Try to get it to build on non-apple internal

[EWS Watchlist]

Attachment 341245 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/testmem/testmem.mm:36:  Extra space between uint8_t and ri_uuid  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/testmem/testmem.mm:36:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 341246 [details]
patch

[Saam Barati]

Created attachment 341247 [details]
patch

[Saam Barati]

Created attachment 341248 [details]
patch

[Saam Barati]

Created attachment 341249 [details]
patch

[Saam Barati]

Comment on attachment 341236 [details]
patch

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

>> PerformanceTests/testmem/air.js:1
>> +'use strict';/*
> 
> I think it wasn't supposed to be here.

yeah this is what the ARES6 cli creates. I can revert this.

[Mark Lam]

Comment on attachment 341249 [details]
patch

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

> Source/JavaScriptCore/testmem/testmem.mm:28
> +#import <libproc.h>

Perhaps you should wrap this in #if __has_include(<libproc.h>) and prefix the $if below with a defined(RUSAGE_INFO_CURRENT) term.

Alternately, move this under the #if RUSAGE_INFO_CURRENT >= 4 && JSC_OBJC_API_ENABLED below, and add a #import <sys/resource.h> here (I think that RUSAGE_INFO_CURRENT is defined there).

[Saam Barati]

Created attachment 341297 [details]
patch

[Saam Barati]

Comment on attachment 341297 [details]
patch

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

> Tools/Scripts/run-testmem:61
> +            print "--count must be > 0\n"

I switched this to "puts" locally

[Saam Barati]

Comment on attachment 341297 [details]
patch

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

> PerformanceTests/testmem/air.js:163
> +/*

I'm going to clean up the multiple copyrights in here.

[Saam Barati]

Created attachment 341299 [details]
patch

[Saam Barati]

Created attachment 341300 [details]
patch

[Saam Barati]

This is the output of run-testmem:
```
air                           
   end: 58.8125 MB
   peak: 59.7734 MB
   time: 525.78 ms
base64
   end: 8.9401 MB
   peak: 8.9701 MB
   time: 148.11 ms
basic
   end: 17.7891 MB
   peak: 17.8594 MB
   time: 370.13 ms
box2d
   end: 40.8307 MB
   peak: 42.8763 MB
   time: 488.65 ms
crypto-md5
   end: 9.651 MB
   peak: 9.651 MB
   time: 58.42 ms
date-format-tofte
   end: 10.9193 MB
   peak: 11.0794 MB
   time: 201.55 ms
earley-boyer
   end: 12.4831 MB
   peak: 12.4844 MB
   time: 55.09 ms
hash-map
   end: 72.8385 MB
   peak: 72.9844 MB
   time: 331.01 ms
regex-dna
   end: 11.4128 MB
   peak: 11.4128 MB
   time: 173.39 ms
splay
   end: 116.0794 MB
   peak: 116.1172 MB
   time: 350.95 ms
tagcloud
   end: 16.4271 MB
   peak: 16.4557 MB
   time: 211.89 ms

end score:           22.628 MB
peak score:          22.8162 MB
total memory score:  22.7219 MB
time score:          212.06 ms
```

I'll probably make it compute confidence intervals at some point. And also the ability to run 2 VMs and compare them at some point too.

[Mark Lam]

Comment on attachment 341300 [details]
patch

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

r=me

> Tools/Scripts/run-testmem:178
> +            print "#{statusToPrint}\r"
> +            scores[path].push(runTest(path, iters))
> +            print "#{" ".rjust(statusToPrint.length)}\r"

Why the strange use of print + "\r" instead of just a plain puts?

[Saam Barati]

Comment on attachment 341300 [details]
patch

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

>> Tools/Scripts/run-testmem:178
>> +            print "#{" ".rjust(statusToPrint.length)}\r"
> 
> Why the strange use of print + "\r" instead of just a plain puts?

It overwrites the same line as the test runs.

[Mark Lam]

Comment on attachment 341300 [details]
patch

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

>>> Tools/Scripts/run-testmem:178
>>> +            print "#{" ".rjust(statusToPrint.length)}\r"
>> 
>> Why the strange use of print + "\r" instead of just a plain puts?
> 
> It overwrites the same line as the test runs.

Nice.  I learned something new.

[WebKit Commit Bot]

Comment on attachment 341300 [details]
patch

Clearing flags on attachment: 341300

Committed r232193: <https://trac.webkit.org/changeset/232193>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40561735>