RESOLVED FIXED 185932
Make a memory test where we can validate JSCs mini memory mode
https://bugs.webkit.org/show_bug.cgi?id=185932
Summary Make a memory test where we can validate JSCs mini memory mode
Saam Barati
Reported 2018-05-23 19:56:37 PDT
...
Attachments
WIP (17.00 KB, patch)
2018-05-23 19:58 PDT, Saam Barati
no flags
patch (1.60 MB, patch)
2018-05-24 17:00 PDT, Saam Barati
no flags
patch (1.60 MB, patch)
2018-05-24 19:58 PDT, Saam Barati
no flags
patch (1.60 MB, patch)
2018-05-24 19:59 PDT, Saam Barati
no flags
patch (1.60 MB, patch)
2018-05-24 20:25 PDT, Saam Barati
no flags
patch (1.60 MB, patch)
2018-05-24 20:51 PDT, Saam Barati
no flags
patch (1.60 MB, patch)
2018-05-24 20:53 PDT, Saam Barati
no flags
patch (1.60 MB, patch)
2018-05-24 21:06 PDT, Saam Barati
no flags
patch (1.60 MB, patch)
2018-05-24 21:13 PDT, Saam Barati
no flags
patch (1.60 MB, patch)
2018-05-25 10:27 PDT, Saam Barati
no flags
patch (1.58 MB, patch)
2018-05-25 10:38 PDT, Saam Barati
no flags
patch (1.58 MB, patch)
2018-05-25 10:44 PDT, Saam Barati
no flags
Saam Barati
Comment 1 2018-05-23 19:58:07 PDT
Saam Barati
Comment 2 2018-05-24 17:00:41 PDT
EWS Watchlist
Comment 3 2018-05-24 17:03:56 PDT
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 4 2018-05-24 17:08:01 PDT
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
Comment 5 2018-05-24 18:02:08 PDT
(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 6 2018-05-24 18:04:03 PDT
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 7 2018-05-24 19:11:06 PDT
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
Comment 8 2018-05-24 19:58:42 PDT
Saam Barati
Comment 9 2018-05-24 19:59:30 PDT
EWS Watchlist
Comment 10 2018-05-24 20:02:38 PDT
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
Comment 11 2018-05-24 20:25:16 PDT
Created attachment 341245 [details] patch Try to get it to build on non-apple internal
EWS Watchlist
Comment 12 2018-05-24 20:27:14 PDT
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
Comment 13 2018-05-24 20:51:47 PDT
Saam Barati
Comment 14 2018-05-24 20:53:56 PDT
Saam Barati
Comment 15 2018-05-24 21:06:05 PDT
Saam Barati
Comment 16 2018-05-24 21:13:13 PDT
Saam Barati
Comment 17 2018-05-24 21:19:23 PDT
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 18 2018-05-25 10:20:41 PDT
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
Comment 19 2018-05-25 10:27:37 PDT
Saam Barati
Comment 20 2018-05-25 10:35:16 PDT
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 21 2018-05-25 10:35:59 PDT
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
Comment 22 2018-05-25 10:38:39 PDT
Saam Barati
Comment 23 2018-05-25 10:44:59 PDT
Saam Barati
Comment 24 2018-05-25 10:46:11 PDT
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 25 2018-05-25 11:02:12 PDT
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 26 2018-05-25 11:25:24 PDT
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 27 2018-05-25 11:29:20 PDT
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 28 2018-05-25 12:05:43 PDT
Comment on attachment 341300 [details] patch Clearing flags on attachment: 341300 Committed r232193: <https://trac.webkit.org/changeset/232193>
WebKit Commit Bot
Comment 29 2018-05-25 12:05:45 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 30 2018-05-25 12:06:39 PDT
Note You need to log in before you can comment on or make changes to this bug.