Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
169743005:
Faster run-bindings-tests (Closed)
|
Created:
6 years, 10 months ago by terry Modified:
6 years, 9 months ago Reviewers:
Nils Barth (inactive) CC:
blink-reviews, kojih, arv+blink, jsbell+bindings_chromium.org, sof, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, haraken, Nate Chapin, Inactive Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionFaster run-bindings-tests
This makes run-bindings-tests faster by doing more work in
Python, instead of going out to the OS.
Notably: calling Python compiler directly (reusing a compiler object),
instead of launching separate processes, and
avoiding a temporary file for interfaces_info.
(Earlier versions had many refactoring changes,
which improve support for Dart CG and other backends,
which Nils broke off and committed separately.)
R=nbarth
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169440
Patch Set 1 #Patch Set 2 : Removed timings and fast is the only mode. #
Total comments: 8
Patch Set 3 : Cleanup #Patch Set 4 : linter cleanup #Patch Set 5 : linter cleanup #Patch Set 6 : Merged and rebased #Patch Set 7 : Integrated CL suggestions #Patch Set 8 : merged #Patch Set 9 : merged #Patch Set 10 : Merged #Patch Set 11 : Another merge #
Total comments: 19
Patch Set 12 : Merged with trunk #Patch Set 13 : minor cleanup #Patch Set 14 : minor cleanup #Patch Set 15 : removed obsolete method #
Total comments: 8
Patch Set 16 : Changes from CR #
Total comments: 21
Patch Set 17 : Integrated CL comments #
Messages
Total messages: 22 (0 generated)
Faster dependency computation and parsing: - Fixed get_file_content being called twice; once for dependencies and again for recording constructors and attributes. - eliminated pickeling of interfaces info - replaced run_command calls with function calls - dynamic PYTHONPATH computation - idl_reader cached for run-binding-tests Also, incorporated Nils suggestions: Nils Barth 2014/02/21 06:00:46 Style: easier to use dict.clear(), as in interfaces_info.clear() (Since not assigning, don't need 'global'.) DONE. Nils Barth 2014/02/21 06:00:46 Probably want to move the IdlReader initialization out and instead pass reader as a parameter, since object construction is presumably heavy (this also constructs a lexer and parser), and doesn't need to be re-done each time. Good idea - DONE. Nils Barth 2014/02/21 06:00:46 I think you can just put these at the top of the module, without a function, directly before the import statement. Yep, moved the computing of the PYTHONPATH dynamically at top of webkitpy/bindings/main.py and moved all imports immediately below that.
Thanks Terry! I'm currently factoring compute_dependencies.py in these 3 CLs: https://codereview.chromium.org/171373006 https://codereview.chromium.org/173503009 https://codereview.chromium.org/174953005 ...so once these land, you'll want to sync to this (which'll be simpler: no constructor IDLs, say). On 2014/02/21 21:25:36, terry wrote: > - eliminated pickeling of interfaces info Could you put this back? Rather, we *need* to pickle interfaces_info for the build, since compute_dependencies and the compiler run as separate processes. It's fine to not do this during r-b-t, but we need it for the real build.
Few more notes. Generally looks fine; once factoring of compute_dependencies lands and we switch the build to Python (Tue/Wed), you can sync to the new Python build and we can do final review. https://codereview.chromium.org/169743005/diff/30001/Source/bindings/scripts/... File Source/bindings/scripts/compute_dependencies.py (left): https://codereview.chromium.org/169743005/diff/30001/Source/bindings/scripts/... Source/bindings/scripts/compute_dependencies.py:78: parser.add_option('--interfaces-info-file', help='output pickle file') We need this separate file for the build, so could you not remove it? In the build, the compute_dependencies step happens before the parallel compile, so we need a separate file. You can see the build in: Source/bindings/generated_bindings.gyp ...and I'll document it at: http://www.chromium.org/developers/design-documents/idl-build https://codereview.chromium.org/169743005/diff/30001/Source/bindings/scripts/... File Source/bindings/scripts/unstable/v8_interface.py (right): https://codereview.chromium.org/169743005/diff/30001/Source/bindings/scripts/... Source/bindings/scripts/unstable/v8_interface.py:69: header_includes = set(INTERFACE_H_INCLUDES) Could you write this as .copy() instead, to make the copying explicit?
Some notes on recent changes. https://codereview.chromium.org/169743005/diff/30001/Source/bindings/scripts/... File Source/bindings/scripts/unstable/idl_compiler.py (right): https://codereview.chromium.org/169743005/diff/30001/Source/bindings/scripts/... Source/bindings/scripts/unstable/idl_compiler.py:65: write_header_and_cpp=code_generator_v8.write_header_and_cpp): I ultimately decided that a code generator object is a good idea for other reasons too (for initialization, say, which should speed it up further); hope this makes it easier! We could standardize the method name if that makes it clearer, or more precisely have an abstract base class (CodeGenerator) and then implement the generate_code() or compile() method in derived classes. CL is: Use a class for CodeGeneratorV8 https://codereview.chromium.org/179423002/ https://codereview.chromium.org/169743005/diff/30001/Source/bindings/scripts/... File Source/bindings/scripts/unstable/v8_interface.py (right): https://codereview.chromium.org/169743005/diff/30001/Source/bindings/scripts/... Source/bindings/scripts/unstable/v8_interface.py:69: header_includes = set(INTERFACE_H_INCLUDES) On 2014/02/24 08:38:56, Nils Barth wrote: > Could you write this as .copy() instead, to make the copying explicit? Actually, I grabbed this in: Use a class for CodeGeneratorV8 https://codereview.chromium.org/179423002/ ...so this should go away on sync. Properly it needs to be done for callback interfaces too. I've made the overall header lists frozenset's to be clear, so the set() is self-explanatory (mutable copy of an immutable variable).
On 2014/02/24 08:34:48, Nils Barth wrote: > Thanks Terry! > > I'm currently factoring compute_dependencies.py in these 3 CLs: > https://codereview.chromium.org/171373006 > https://codereview.chromium.org/173503009 > https://codereview.chromium.org/174953005 > ...so once these land, you'll want to sync to this (which'll be simpler: no > constructor IDLs, say). > > > On 2014/02/21 21:25:36, terry wrote: > > - eliminated pickeling of interfaces info > > Could you put this back? > > Rather, we *need* to pickle interfaces_info for the build, since > compute_dependencies > and the compiler run as separate processes. > It's fine to not do this during r-b-t, but we need it for the real build. interfaces_info is back being pickled.
Nils, I've sync'd to you changes and made the changes you've suggested. PTAL. Thanks again. https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... File Source/bindings/scripts/compute_dependencies.py (left): https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... Source/bindings/scripts/compute_dependencies.py:78: parser.add_option('--interfaces-info-file', help='output pickle file') On 2014/02/24 08:38:56, Nils Barth wrote: > We need this separate file for the build, so could you not remove it? > In the build, the compute_dependencies step happens before the parallel compile, > so we need a separate file. > > You can see the build in: > Source/bindings/generated_bindings.gyp > ...and I'll document it at: > http://www.chromium.org/developers/design-documents/idl-build Done. https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... File Source/bindings/scripts/unstable/idl_compiler.py (right): https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... Source/bindings/scripts/unstable/idl_compiler.py:65: write_header_and_cpp=code_generator_v8.write_header_and_cpp): This works for me right now. Maybe an abstract case would be nicer but this is fine for now. On 2014/02/25 07:29:59, Nils Barth wrote: > I ultimately decided that a code generator object is a good idea > for other reasons too (for initialization, say, which should speed > it up further); hope this makes it easier! > > We could standardize the method name if that makes it clearer, > or more precisely have an abstract base class (CodeGenerator) > and then implement the generate_code() or compile() method in > derived classes. > > CL is: > Use a class for CodeGeneratorV8 > https://codereview.chromium.org/179423002/ https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... File Source/bindings/scripts/unstable/v8_interface.py (right): https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... Source/bindings/scripts/unstable/v8_interface.py:69: header_includes = set(INTERFACE_H_INCLUDES) Thanks, frozenset is much nicer. On 2014/02/25 07:29:59, Nils Barth wrote: > On 2014/02/24 08:38:56, Nils Barth wrote: > > Could you write this as .copy() instead, to make the copying explicit? > > Actually, I grabbed this in: > Use a class for CodeGeneratorV8 > https://codereview.chromium.org/179423002/ > ...so this should go away on sync. > > Properly it needs to be done for callback interfaces too. > I've made the overall header lists frozenset's to be clear, > so the set() is self-explanatory (mutable copy of an immutable variable).
Nils I've re-merged with your latest round of checkins (removed more code as you moved to less files - yay!). PTAL. On 2014/02/27 23:40:25, terry wrote: > Nils, > > I've sync'd to you changes and made the changes you've suggested. PTAL. > > Thanks again. > > https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... > File Source/bindings/scripts/compute_dependencies.py (left): > > https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... > Source/bindings/scripts/compute_dependencies.py:78: > parser.add_option('--interfaces-info-file', help='output pickle file') > On 2014/02/24 08:38:56, Nils Barth wrote: > > We need this separate file for the build, so could you not remove it? > > In the build, the compute_dependencies step happens before the parallel > compile, > > so we need a separate file. > > > > You can see the build in: > > Source/bindings/generated_bindings.gyp > > ...and I'll document it at: > > http://www.chromium.org/developers/design-documents/idl-build > > Done. > > https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... > File Source/bindings/scripts/unstable/idl_compiler.py (right): > > https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... > Source/bindings/scripts/unstable/idl_compiler.py:65: > write_header_and_cpp=code_generator_v8.write_header_and_cpp): > This works for me right now. Maybe an abstract case would be nicer but this is > fine for now. > > On 2014/02/25 07:29:59, Nils Barth wrote: > > I ultimately decided that a code generator object is a good idea > > for other reasons too (for initialization, say, which should speed > > it up further); hope this makes it easier! > > > > We could standardize the method name if that makes it clearer, > > or more precisely have an abstract base class (CodeGenerator) > > and then implement the generate_code() or compile() method in > > derived classes. > > > > CL is: > > Use a class for CodeGeneratorV8 > > https://codereview.chromium.org/179423002/ > > https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... > File Source/bindings/scripts/unstable/v8_interface.py (right): > > https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... > Source/bindings/scripts/unstable/v8_interface.py:69: header_includes = > set(INTERFACE_H_INCLUDES) > Thanks, frozenset is much nicer. > On 2014/02/25 07:29:59, Nils Barth wrote: > > On 2014/02/24 08:38:56, Nils Barth wrote: > > > Could you write this as .copy() instead, to make the copying explicit? > > > > Actually, I grabbed this in: > > Use a class for CodeGeneratorV8 > > https://codereview.chromium.org/179423002/ > > ...so this should go away on sync. > > > > Properly it needs to be done for callback interfaces too. > > I've made the overall header lists frozenset's to be clear, > > so the set() is self-explanatory (mutable copy of an immutable variable).
Thanks for revisions Terry! Main change I'd suggest is to have an IdlCompiler class to consolidate the (verbose) initialization; that makes the calls very clean. Other than that, a few minor tweaks. Now that we've switched to Python, it's fine to commit this CL once we've finished revising it! https://codereview.chromium.org/169743005/diff/270001/Source/bindings/__init_... File Source/bindings/__init__.py (right): https://codereview.chromium.org/169743005/diff/270001/Source/bindings/__init_... Source/bindings/__init__.py:1: # Required for Python to search this directory for module files This comment is unnecessary: empty files are fine, as empty __init__.py is standard in Python. (Thanks though!) https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... File Source/bindings/scripts/__init__.py (right): https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/__init__.py:1: # Required for Python to search this directory for module files Here too, no comment necessary (empty file ok). https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... File Source/bindings/scripts/compute_interfaces_info.py (right): https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/compute_interfaces_info.py:353: extended_attributes_by_interface.clear() # interface name -> extended attributes Could you remove the comment at the end of the line here? (It's just needed on the initialization line.) https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/compute_interfaces_info.py:359: Could you remove this blank line, as the function will soon be only 3 lines long? https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/compute_interfaces_info.py:362: write_dependencies_file(interface_dependencies_file, only_if_changed) BTW, I'm removing the interface_dependencies_file right now, and I'll remove the event_names_file in a day or so, so this will soon be 3 lines long! https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... File Source/bindings/scripts/idl_compiler.py (right): https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/idl_compiler.py:78: def compile_idl(idl_filename, output_directory, idl_attributes_file, Initialization makes this a bit ugly. Let's make the compiler into a class, that way we have one initialization, and it's a very simple call to write the file. You can then either pass a different parameter in the initialization, or derive a class and override the methods (or maybe both if that's easier). How about: class IdlCompiler(object): def __init__(self, output_directory, interface_dependencies_file, interfaces_info_file=None, only_if_changed=False, reader=None, code_generator=None): self.output_directory = output_directory # interfaces_info initialization self.reader = reader or IdlReader(interfaces_info, idl_attributes_file, output_directory) self.code_generator = code_generator or CodeGeneratorV8(interfaces_info, output_directory) def compile_idl(self, idl_filename): # compile and write https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/idl_compiler.py:83: output_directory = output_directory I don't think this line is necessary ;) https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/idl_compiler.py:96: cg = code_generator(interfaces_info, output_directory) In Blink we use full words for names, so |code_generator|, not |cg|. (Admittedly, C/Go-style |r| for reader and |cg| for |code_generator| would be fun ;) https://codereview.chromium.org/169743005/diff/270001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/bindings/main.py (right): https://codereview.chromium.org/169743005/diff/270001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/bindings/main.py:115: self.reader = None If we make the compiler an object, this becomes self.compiler instead. Also, we should do the compiler initialization here; that makes the actual compilation v. simple. https://codereview.chromium.org/169743005/diff/270001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/bindings/main.py:167: def list_idl_file(idl_paths): This is unnecessary, right? (It's just copying a list.) Can we just remove this function and use the results of idl_paths() and idl_paths_recursive() directly? (Anyway, nice move skipping a file write/read!)
Quick update on m0ar simplifications. https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... File Source/bindings/scripts/compute_dependencies.py (left): https://chromiumcodereview.appspot.com/169743005/diff/30001/Source/bindings/s... Source/bindings/scripts/compute_dependencies.py:78: parser.add_option('--interfaces-info-file', help='output pickle file') On 2014/02/27 23:40:26, terry wrote: > On 2014/02/24 08:38:56, Nils Barth wrote: > > We need this separate file for the build, so could you not remove it? > > In the build, the compute_dependencies step happens before the parallel > compile, > > so we need a separate file. > > > > You can see the build in: > > Source/bindings/generated_bindings.gyp > > ...and I'll document it at: > > http://www.chromium.org/developers/design-documents/idl-build > > Done. Just to clarify: it's fine to not write InterfacesInfo.pickle in r-b-t, as you suggest, but we do need it in the build. https://chromiumcodereview.appspot.com/169743005/diff/270001/Source/bindings/... File Source/bindings/scripts/compute_interfaces_info.py (right): https://chromiumcodereview.appspot.com/169743005/diff/270001/Source/bindings/... Source/bindings/scripts/compute_interfaces_info.py:356: def compute(idl_files, interfaces_info_file, interface_dependencies_file, This function (and clear_globals()) won't be necessary once I move EventInterfaces.in out of compute_interfaces_info (and remove the duplicate test)! https://chromiumcodereview.appspot.com/169743005/diff/270001/Source/bindings/... File Source/bindings/scripts/idl_compiler.py (right): https://chromiumcodereview.appspot.com/169743005/diff/270001/Source/bindings/... Source/bindings/scripts/idl_compiler.py:78: def compile_idl(idl_filename, output_directory, idl_attributes_file, On 2014/03/03 01:32:48, Nils Barth wrote: > Initialization makes this a bit ugly. > Let's make the compiler into a class, that way we have one initialization, > and it's a very simple call to write the file. This is useful enough that I'm posting it in another CL (just makes the class). Make an IdlCompiler class https://codereview.chromium.org/178003010/ For hooking in Dart, you'll just need to add override fields for the reader/CG, or simply monkey patch it after initialization. https://chromiumcodereview.appspot.com/169743005/diff/270001/Tools/Scripts/we... File Tools/Scripts/webkitpy/bindings/main.py (right): https://chromiumcodereview.appspot.com/169743005/diff/270001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:173: def compute_interfaces_info(idl_files_list_filename, I'm simplifying this (remove EventInterfaces.in), and going to remove the multiple calls, so you'll just need to have: self.interfaces_info = compute_interfaces_info(idl_files)
FYI: I've made substantial simplifications to compute_interfaces_info.py and r-b-t (main.py) in 2 CLs (below). Once they land and you sync to these changes, this CL should become v. simple. (I'll ping you when they land.) You'll mostly just need to instantiate a compiler in the test constructor: interfaces_info = compute_interfaces_info(idl_files) self.compiler = IdlCompiler(output_dir, interfaces_info, idl_attributes_file) ...and then just call the compiler repeatedly! CLs waiting to land: Clean up generate_event_interfaces.py https://codereview.chromium.org/186433003/ Clean up compute_interfaces_info.py https://codereview.chromium.org/183853024/
*ping* Refactoring of IDL build is complete, so a rebase should simplify this CL, and it should be good to go! There's one more CL landing: Factor abstract base class IDLCompiler, concrete class IDLComplierV8 https://codereview.chromium.org/189543008/ ...which should make reuse by Dart (and Closure, and others) simpler, but that shouldn't block this one.
All relevant refactoring CLs have landed, so once this CL is synced, it'll be quite simple and should be quick to review! (Thanks for all the good ideas that came out of this too!)
Nils, I've merged with your latest changes. The diffs are now minor. PTAL. https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... File Source/bindings/scripts/compute_interfaces_info.py (right): https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/compute_interfaces_info.py:353: extended_attributes_by_interface.clear() # interface name -> extended attributes On 2014/03/03 01:32:48, Nils Barth wrote: > Could you remove the comment at the end of the line here? > (It's just needed on the initialization line.) Done. https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/compute_interfaces_info.py:359: On 2014/03/03 01:32:48, Nils Barth wrote: > Could you remove this blank line, as the function will soon be only 3 lines > long? Done. https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/compute_interfaces_info.py:362: write_dependencies_file(interface_dependencies_file, only_if_changed) On 2014/03/03 01:32:48, Nils Barth wrote: > BTW, I'm removing the interface_dependencies_file right now, > and I'll remove the event_names_file in a day or so, > so this will soon be 3 lines long! Done. https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... File Source/bindings/scripts/idl_compiler.py (right): https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/idl_compiler.py:78: def compile_idl(idl_filename, output_directory, idl_attributes_file, Right - you've added the abstract class. On 2014/03/03 01:32:48, Nils Barth wrote: > Initialization makes this a bit ugly. > Let's make the compiler into a class, that way we have one initialization, > and it's a very simple call to write the file. > > You can then either pass a different parameter in the initialization, > or derive a class and override the methods > (or maybe both if that's easier). > > How about: > class IdlCompiler(object): > def __init__(self, output_directory, interface_dependencies_file, > interfaces_info_file=None, only_if_changed=False, reader=None, > code_generator=None): > self.output_directory = output_directory > # interfaces_info initialization > self.reader = reader or IdlReader(interfaces_info, idl_attributes_file, > output_directory) > self.code_generator = code_generator or CodeGeneratorV8(interfaces_info, > output_directory) > > def compile_idl(self, idl_filename): > # compile and write https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/idl_compiler.py:83: output_directory = output_directory On 2014/03/03 01:32:48, Nils Barth wrote: > I don't think this line is necessary ;) Not necessary since self.output_directory is set on init. https://codereview.chromium.org/169743005/diff/270001/Source/bindings/scripts... Source/bindings/scripts/idl_compiler.py:96: cg = code_generator(interfaces_info, output_directory) ok. On 2014/03/03 01:32:48, Nils Barth wrote: > In Blink we use full words for names, so |code_generator|, not |cg|. > (Admittedly, C/Go-style |r| for reader and |cg| for |code_generator| would be > fun ;)
One revision, which makes this even simpler: we don't need a temporary file for |interfaces_info|, since we can just pass the dict directly! https://codereview.chromium.org/169743005/diff/350001/Source/bindings/scripts... File Source/bindings/scripts/compute_interfaces_info.py (right): https://codereview.chromium.org/169743005/diff/350001/Source/bindings/scripts... Source/bindings/scripts/compute_interfaces_info.py:271: def compute(idl_files, interfaces_info_file, only_if_changed): This won't be necessary (indeed, no changes to this file are necessary) if we just pass the interfaces_info dict directly. https://codereview.chromium.org/169743005/diff/350001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/bindings/main.py (right): https://codereview.chromium.org/169743005/diff/350001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/bindings/main.py:43: from bindings.scripts.compute_interfaces_info import compute Here you can also import |interfaces_info| ...to access the module variable. https://codereview.chromium.org/169743005/diff/350001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/bindings/main.py:160: compute(idl_files_list_filename, self.interfaces_info_filename, False) Instead of using an intermediate file, we can just call the function as you suggested. Thus this would just be: compute_interfaces_info(idl_files) (We could use a return value, but that's not necessary.) ...and in fact this is just one line now, so we can inline it. https://codereview.chromium.org/169743005/diff/350001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/bindings/main.py:238: interfaces_info_filename=self.interfaces_info_filename, We can pass the dict in directly (no file needed!) as: interfaces_info=interfaces_info, (assuming you've imported it into this context).
PTAL https://chromiumcodereview.appspot.com/169743005/diff/350001/Source/bindings/... File Source/bindings/scripts/compute_interfaces_info.py (right): https://chromiumcodereview.appspot.com/169743005/diff/350001/Source/bindings/... Source/bindings/scripts/compute_interfaces_info.py:271: def compute(idl_files, interfaces_info_file, only_if_changed): Right with compute_interfaces_info no need for this function. On 2014/03/14 01:01:37, Nils Barth wrote: > This won't be necessary (indeed, no changes to this file are necessary) > if we just pass the interfaces_info dict directly. https://chromiumcodereview.appspot.com/169743005/diff/350001/Tools/Scripts/we... File Tools/Scripts/webkitpy/bindings/main.py (right): https://chromiumcodereview.appspot.com/169743005/diff/350001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:43: from bindings.scripts.compute_interfaces_info import compute Done. On 2014/03/14 01:01:37, Nils Barth wrote: > Here you can also import |interfaces_info| > ...to access the module variable. https://chromiumcodereview.appspot.com/169743005/diff/350001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:160: compute(idl_files_list_filename, self.interfaces_info_filename, False) Right, with the compiler taking interface_info I'll do just that. On 2014/03/14 01:01:37, Nils Barth wrote: > Instead of using an intermediate file, we can just call the function as you > suggested. > Thus this would just be: > compute_interfaces_info(idl_files) > > (We could use a return value, but that's not necessary.) > > ...and in fact this is just one line now, so we can inline it. https://chromiumcodereview.appspot.com/169743005/diff/350001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:238: interfaces_info_filename=self.interfaces_info_filename, Good idea. I didn't noticed you had added that in the compiler (great). Done. On 2014/03/14 01:01:37, Nils Barth wrote: > We can pass the dict in directly (no file needed!) as: > interfaces_info=interfaces_info, > (assuming you've imported it into this context).
Thanks Terry! A few style nits, and I just noticed that we need to update the exception handling, but otherwise it's fine. A few tweaks and we should be good to go! https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... File Tools/Scripts/webkitpy/bindings/main.py (right): https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:36: # for compute_dependencies and idl_compiler Could you update this to: "compute_interfaces_info" https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:38: source_path = os.path.normpath(os.path.join(module_path, os.pardir, Could you wrap the line slightly tighter? It fits as: module_path, os.pardir, os.pardir, os.pardir, os.pardir, 'Source' ...in detail: source_path = os.path.normpath(os.path.join(module_path, os.pardir, os.pardir, os.pardir, os.pardir, 'Source')) https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:129: idl_file_fullpath = os.path.realpath(idl_file) Could you move this before the try statement? https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:131: except ScriptError, e: We need to update this catch clause, since we're no longer calling an external script. Could you make this: except Exception as err: (so we catch all exceptions) https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:133: print e.output This can then just be: print err https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:134: return e.exit_code ...and then: return 1 https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:171: except ScriptError, e: Could you update this catch clause as described above? https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:198: except ScriptError, e: BTW, could you update this to: except ScriptError as err: ...as that's more modern syntax? Thanks! https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:233: self.idl_compiler = IdlCompilerV8(self.output_directory, Could you move this up into the __init__? https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:272: return -1 Also, could we update this to return 1 ? (Exit status should be in range 0-255.)
Thanks Nils, PTAL. https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... File Tools/Scripts/webkitpy/bindings/main.py (right): https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:36: # for compute_dependencies and idl_compiler On 2014/03/17 02:06:14, Nils Barth wrote: > Could you update this to: "compute_interfaces_info" Done. https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:38: source_path = os.path.normpath(os.path.join(module_path, os.pardir, On 2014/03/17 02:06:14, Nils Barth wrote: > Could you wrap the line slightly tighter? > It fits as: > module_path, os.pardir, os.pardir, > os.pardir, os.pardir, 'Source' > > ...in detail: > source_path = os.path.normpath(os.path.join(module_path, os.pardir, os.pardir, > os.pardir, os.pardir, 'Source')) Done. https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:129: idl_file_fullpath = os.path.realpath(idl_file) On 2014/03/17 02:06:14, Nils Barth wrote: > Could you move this before the try statement? Done. https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:131: except ScriptError, e: On 2014/03/17 02:06:14, Nils Barth wrote: > We need to update this catch clause, > since we're no longer calling an external script. > Could you make this: > except Exception as err: > (so we catch all exceptions) Done. https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:133: print e.output On 2014/03/17 02:06:14, Nils Barth wrote: > This can then just be: > print err Done. https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:134: return e.exit_code On 2014/03/17 02:06:14, Nils Barth wrote: > ...and then: > return 1 Done. https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:171: except ScriptError, e: On 2014/03/17 02:06:14, Nils Barth wrote: > Could you update this catch clause as described above? Done. https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:198: except ScriptError, e: On 2014/03/17 02:06:14, Nils Barth wrote: > BTW, could you update this to: > except ScriptError as err: > ...as that's more modern syntax? > > Thanks! Done. https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:233: self.idl_compiler = IdlCompilerV8(self.output_directory, On 2014/03/17 02:06:14, Nils Barth wrote: > Could you move this up into the __init__? I don't think so the interfaces_info isn't computed yet. Even though the interfaces_info is a dict doesn't seem to get the interfaces_info which is mutated. If IdlCompilerV8 is called after the call to compute_interfaces_info is called (after __init__ being called) then all is okay. Would also need to move current_scm = detect_scm_system(os.curdir) os.chdir(os.path.join(current_scm.checkout_root, 'Source')) to __init__ https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:272: return -1 On 2014/03/17 02:06:14, Nils Barth wrote: > Also, could we update this to > return 1 > ? > (Exit status should be in range 0-255.) Done.
LGTM, thanks! https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... File Tools/Scripts/webkitpy/bindings/main.py (right): https://chromiumcodereview.appspot.com/169743005/diff/370001/Tools/Scripts/we... Tools/Scripts/webkitpy/bindings/main.py:233: self.idl_compiler = IdlCompilerV8(self.output_directory, On 2014/03/17 16:36:38, terry wrote: > On 2014/03/17 02:06:14, Nils Barth wrote: > > Could you move this up into the __init__? > > I don't think so the interfaces_info isn't computed yet. Got it, thanks! (I think we could simplify r-b-t by just having a function, instead of an object we only use once, but that's a bigger change.)
The CQ bit was checked by nbarth@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/terry@google.com/169743005/380001
Message was sent while issue was closed.
Change committed as 169440
Message was sent while issue was closed.
On 2014/03/18 10:15:52, I haz the power (commit-bot) wrote: > Change committed as 169440 Thanks Terry! This is great -- cuts r-b-t time down to 1.4 seconds for me (from over 3 seconds), with user time ~1s, and sys time .06 seconds (down from .48 seconds)! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
