Speed up fremontcut/dartdomgenerator

Speed up fremontcut/dartdomgenerator

- Parse IDL in parallel
- Support calling fremontcut from dartdomgenerator to avoid reading database

This speeds up the whole go.sh by about 4x on my machine.  Looks like the remaining bottleneck is in the emitter.

Committed: https://code.google.com/p/dart/source/detail?r=13038

[Anton Muhin, 2012-09-26 06:37:29]

That's really awesome!

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
File lib/html/scripts/databasebuilder.py (right):

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:76: idl_ast = idl_parser.parse(content,
nit: unless it's against the style, maybe idl_parser.parse(\ncontent, defines=)
as you're touching this code anyway?

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:81: raise RuntimeError('Failed to load file
%s: %s' % (file_name, e))
in multiprocessing, what would be result of throwing an exception from child
process?

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:120: new_name = rename(idl_node.id)
why this change?  does that belong to this CL?

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:338: node.doc_js_interface_name =
old_interface.id
ditto

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:448: result = result_queue.get()
same question as above: if some of child processes die for any reason, will it
throw an exception or we'll block forever?

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:451: def _process_idl_file(self, idl_file,
should it be a method on its own or it might be a closure?  And if it can be a
closure, is it required for import_options to be an optional argument?

[vsm, 2012-09-26 16:22:10]

thanks, PTAL

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
File lib/html/scripts/databasebuilder.py (right):

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:76: idl_ast = idl_parser.parse(content,
On 2012/09/26 06:37:29, Anton Muhin wrote:
> nit: unless it's against the style, maybe idl_parser.parse(\ncontent,
defines=)
> as you're touching this code anyway?

Done.

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:81: raise RuntimeError('Failed to load file
%s: %s' % (file_name, e))
It'll quite the process, but the code below would hang waiting for the
result_queue.  I've modified to make the code more robust and tested with some
IDL errors to make sure they're propagated correctly.

On 2012/09/26 06:37:29, Anton Muhin wrote:
> in multiprocessing, what would be result of throwing an exception from child
> process?

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:120: new_name = rename(idl_node.id)
It turns out that writing a database to file and reading it back in was not
idempotent.  I'm trying to fix part of that here.  Before, these js names were
not renamed properly, but if the database was written and read, they'd be
properly reset to idl_name.id so it worked anyway.

On 2012/09/26 06:37:29, Anton Muhin wrote:
> why this change?  does that belong to this CL?

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:338: node.doc_js_interface_name =
old_interface.id
Same issue as above.

On 2012/09/26 06:37:29, Anton Muhin wrote:
> ditto

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:448: result = result_queue.get()
PTAL.  I've tested both bad IDL and timeout cases (by setting the timeout to 0).
 It quits and cleans up dead processes in both cases.

On 2012/09/26 06:37:29, Anton Muhin wrote:
> same question as above: if some of child processes die for any reason, will it
> throw an exception or we'll block forever?

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:451: def _process_idl_file(self, idl_file,
On 2012/09/26 06:37:29, Anton Muhin wrote:
> should it be a method on its own or it might be a closure?  And if it can be a
> closure, is it required for import_options to be an optional argument?

I've dropped the optional argument.  It's not called anywhere else, but I think
the import function above is a little more readable if I take this out.

[vsm, 2012-09-26 16:23:41]

BTW, to be clear, I haven't tested this on windows, but multiprocessing is
supposed to work there as well.

[Anton Muhin, 2012-09-26 17:53:09]

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
File lib/html/scripts/databasebuilder.py (right):

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:120: new_name = rename(idl_node.id)
Weird.  Might be worth splitting those change if things will go weird on
windows.

Up to you to decide though.

On 2012/09/26 16:22:10, vsm wrote:
> It turns out that writing a database to file and reading it back in was not
> idempotent.  I'm trying to fix part of that here.  Before, these js names were
> not renamed properly, but if the database was written and read, they'd be
> properly reset to idl_name.id so it worked anyway.
> 
> On 2012/09/26 06:37:29, Anton Muhin wrote:
> > why this change?  does that belong to this CL?
>

https://chromiumcodereview.appspot.com/10987042/diff/8002/lib/html/scripts/da...
File lib/html/scripts/dartdomgenerator.py (right):

https://chromiumcodereview.appspot.com/10987042/diff/8002/lib/html/scripts/da...
lib/html/scripts/dartdomgenerator.py:182: GenerateFromDatabase(database,
dart2js_output_dir,
nit: fits single line?

https://chromiumcodereview.appspot.com/10987042/diff/8002/lib/html/scripts/da...
File lib/html/scripts/databasebuilder.py (right):

https://chromiumcodereview.appspot.com/10987042/diff/8002/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:83: result_queue.put(RuntimeError('Failed to
load file %s: %s' % (file_name, e)),
nit: is line too long?  I am personally fine with it, so feel free to ignore

https://chromiumcodereview.appspot.com/10987042/diff/8002/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:130: if isinstance(idl_node, IDLInterface):
up to you, but might be more natural to have a method on IDLNode which is
overridden on IDLInterface.

https://chromiumcodereview.appspot.com/10987042/diff/8002/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:131: idl_node.doc_js_name = new_name
I am not familiar with this part of the system, but new_name looks like name
dart:html lib, judging from name.  If that's true, resetting
doc_js_name/javascript_binding_name with it looks a bit weird.

https://chromiumcodereview.appspot.com/10987042/diff/8002/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:465: job.terminate()
if you just terminate the parent process, will it terminate all the children
processes?

And can job.start() fail?  If yes, you guard is apparently not enough.

[Anton Muhin, 2012-09-26 17:57:26]

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
File lib/html/scripts/databasebuilder.py (right):

https://chromiumcodereview.appspot.com/10987042/diff/2001/lib/html/scripts/da...
lib/html/scripts/databasebuilder.py:451: def _process_idl_file(self, idl_file,
I'd rather make it a closure of the only caller, but it's your call

On 2012/09/26 16:22:10, vsm wrote:
> On 2012/09/26 06:37:29, Anton Muhin wrote:
> > should it be a method on its own or it might be a closure?  And if it can be
a
> > closure, is it required for import_options to be an optional argument?
> 
> I've dropped the optional argument.  It's not called anywhere else, but I
think
> the import function above is a little more readable if I take this out.

[vsm, 2012-09-28 16:17:25]

PTAL

https://codereview.chromium.org/10987042/diff/8002/lib/html/scripts/dartdomge...
File lib/html/scripts/dartdomgenerator.py (right):

https://codereview.chromium.org/10987042/diff/8002/lib/html/scripts/dartdomge...
lib/html/scripts/dartdomgenerator.py:182: GenerateFromDatabase(database,
dart2js_output_dir,
On 2012/09/26 17:53:09, Anton Muhin wrote:
> nit: fits single line?

Done.

https://codereview.chromium.org/10987042/diff/8002/lib/html/scripts/databaseb...
File lib/html/scripts/databasebuilder.py (right):

https://codereview.chromium.org/10987042/diff/8002/lib/html/scripts/databaseb...
lib/html/scripts/databasebuilder.py:83: result_queue.put(RuntimeError('Failed to
load file %s: %s' % (file_name, e)),
On 2012/09/26 17:53:09, Anton Muhin wrote:
> nit: is line too long?  I am personally fine with it, so feel free to ignore

Done.

https://codereview.chromium.org/10987042/diff/8002/lib/html/scripts/databaseb...
lib/html/scripts/databasebuilder.py:130: if isinstance(idl_node, IDLInterface):
On 2012/09/26 17:53:09, Anton Muhin wrote:
> up to you, but might be more natural to have a method on IDLNode which is
> overridden on IDLInterface.

Done.

https://codereview.chromium.org/10987042/diff/8002/lib/html/scripts/databaseb...
lib/html/scripts/databasebuilder.py:131: idl_node.doc_js_name = new_name
It's a little confusing, but the renaming during database building is about
normalization (DOMCoreException to DOMException).  It should make things closer
to the right name.

On 2012/09/26 17:53:09, Anton Muhin wrote:
> I am not familiar with this part of the system, but new_name looks like name
> dart:html lib, judging from name.  If that's true, resetting
> doc_js_name/javascript_binding_name with it looks a bit weird.

https://codereview.chromium.org/10987042/diff/8002/lib/html/scripts/databaseb...
lib/html/scripts/databasebuilder.py:465: job.terminate()
Not clear from the docs if start can throw.  I've moved it into the try anyway.

The child processes do not call any blocking APIs now, so it should not block. 
On linux, I tried killing the parent and the child processes do not appear to
hang around.

On 2012/09/26 17:53:09, Anton Muhin wrote:
> if you just terminate the parent process, will it terminate all the children
> processes?
> 
> And can job.start() fail?  If yes, you guard is apparently not enough.

[Anton Muhin, 2012-09-28 18:01:36]

STV!

That's really great to see speed up in this part of pipeline