We shouldn't use those fancy index sparse sets and vectors and stuff. We should just use RegisterSet, since that's a soooper fast and compact bitvector.
Created attachment 305428 [details] maybe it'll work
Created attachment 305434 [details] the patch
Attachment 305434 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3TimingScope.cpp:60: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/b3/air/AirLowerAfterRegAlloc.cpp:37: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.h:45: The parameter name "code" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/air/AirReportUsedRegisters.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:52: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:61: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:64: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:68: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:79: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:86: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:104: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:114: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 12 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 305434 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=305434&action=review r=me > Source/JavaScriptCore/b3/B3TimingScope.cpp:60 > + return ensurePointer(s_state, [] { return new State(); }); Nice thing! > Source/JavaScriptCore/b3/air/AirLiveness.h:-403 > -typedef AbstractLiveness<RegLivenessAdapter> RegLiveness; OK, this will be replaced with specialized RegLiveness, which uses RegisterSet. > Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:95 > +} OK, it is (logically) identical to the AbstractLiveness one.
Landed in https://trac.webkit.org/changeset/214408/webkit
Comment on attachment 305434 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=305434&action=review > Source/JavaScriptCore/ChangeLog:10 > + AbstractLiveness<>, but it's about 30% faster. It gets its speed boost from just using Awesome! > Source/WTF/wtf/Atomics.h:532 > + T* newValue = func(); Style nit: It feels weird to take a lambda here, and assume that it calls "new" on T, since you call "delete" below. Why not just take arguments for T's constructor that you forward to it?
Comment on attachment 305434 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=305434&action=review >> Source/WTF/wtf/Atomics.h:532 >> + T* newValue = func(); > > Style nit: It feels weird to take a lambda here, and assume that it calls "new" on T, since you call "delete" below. Why not just take arguments for T's constructor that you forward to it? Or perhaps have two lambdas. One for making a value, one for cleanup
(In reply to Saam Barati from comment #7) > Comment on attachment 305434 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=305434&action=review > > >> Source/WTF/wtf/Atomics.h:532 > >> + T* newValue = func(); > > > > Style nit: It feels weird to take a lambda here, and assume that it calls "new" on T, since you call "delete" below. Why not just take arguments for T's constructor that you forward to it? > > Or perhaps have two lambdas. One for making a value, one for cleanup You're right, the lambda is dumb. It should just take a type parameter and new it, and then it should take an argument pack to pass to the constructor.