It would be nice to be able to track memory usage over commits to check for regressions.
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.
Don, to integrate this benchmark in the playstation port, you'll need to implement the "struct MemoryFootprint" inside jsc.cpp.
<rdar://problem/53990898>
Created attachment 375637 [details] Patch
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.
(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.
Created attachment 375647 [details] Updated patch responding to review comments
LGTM after updates. Unofficial r+.
(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 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.
Committed r248326: <https://trac.webkit.org/changeset/248326>
Thank you for this. It is really useful.