WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
patch
(1.60 MB, patch)
2018-05-24 17:00 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(1.60 MB, patch)
2018-05-24 19:58 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(1.60 MB, patch)
2018-05-24 19:59 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(1.60 MB, patch)
2018-05-24 20:25 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(1.60 MB, patch)
2018-05-24 20:51 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(1.60 MB, patch)
2018-05-24 20:53 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(1.60 MB, patch)
2018-05-24 21:06 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(1.60 MB, patch)
2018-05-24 21:13 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(1.60 MB, patch)
2018-05-25 10:27 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(1.58 MB, patch)
2018-05-25 10:38 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
patch
(1.58 MB, patch)
2018-05-25 10:44 PDT
,
Saam Barati
no flags
Details
Formatted Diff
Diff
Show Obsolete
(11)
View All
Add attachment
proposed patch, testcase, etc.
Saam Barati
Comment 1
2018-05-23 19:58:07 PDT
Created
attachment 341161
[details]
WIP
Saam Barati
Comment 2
2018-05-24 17:00:41 PDT
Created
attachment 341236
[details]
patch
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
Created
attachment 341241
[details]
patch
Saam Barati
Comment 9
2018-05-24 19:59:30 PDT
Created
attachment 341242
[details]
patch
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
Created
attachment 341246
[details]
patch
Saam Barati
Comment 14
2018-05-24 20:53:56 PDT
Created
attachment 341247
[details]
patch
Saam Barati
Comment 15
2018-05-24 21:06:05 PDT
Created
attachment 341248
[details]
patch
Saam Barati
Comment 16
2018-05-24 21:13:13 PDT
Created
attachment 341249
[details]
patch
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
Created
attachment 341297
[details]
patch
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
Created
attachment 341299
[details]
patch
Saam Barati
Comment 23
2018-05-25 10:44:59 PDT
Created
attachment 341300
[details]
patch
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
<
rdar://problem/40561735
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug