IDL compiler: Pickle interface information

IDL compiler: Pickle interface information

This CL writes IDL interface dependency information to a pickle file
(Python serialization), Interfaces.pickle.
This is for speed and simplicity of handling dependencies between interfaces
and global interface information.

1. Serializing/Deserializing a pickle is v. fast (done in optimized C);
　we don't need to manually parse a custom text file format
　(also simplifies the code b/c don't need custom read/write functions).
2. For implementing IDL compiler features that depend on inspecting other
　interfaces, storing global information in this pickle speeds up compilation
　b/c we don't need to re-read and re-parse dependency files.

For example, to support [ImplementedAs], currently Perl has to parse
the IDL file for every interface type that is used, just to check if
it has an [ImplementedAs]. This is a major cause of slowness in the Perl
compiler.
Instead, in a followup CL I plan to store this [ImplementedAs] data
in Interfaces.pickle, so it's immediately available from reading this
one file.

This CL just adds the pickle file and does some simplification of the Python;
further refactoring will be possible once we remove Perl
and can remove the legacy text file, but we can do that later,
as part of rewriting dependency resolution.

BUG=239771
R=haraken

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164570

[Nils Barth (inactive), 2014-01-06 06:02:10]

Here's a preliminary "inter-IDL features" CL;
using a pickle for global information, we can easily add
v. fast support for [ImplementedAs] and callback interfaces on types
(main remaining feature for IDL compiler)!

[haraken, 2014-01-06 06:16:49]

I'm a bit afraid that adding the intermediate file (Interfaces.pickle) might
make it harder to optimize the compiler. If we have Interfaces.pickle, it
produces a dependency:

- any IDL file => Interfaces.pickle
- Interfaces.pickle => all generated C++ files

This implies that we have to rebuild all C++ files when any IDL file is changed.

What's your plan about this?

[Nils Barth (inactive), 2014-01-06 06:38:46]

On 2014/01/06 06:16:49, haraken wrote:
> I'm a bit afraid that adding the intermediate file (Interfaces.pickle) might
> make it harder to optimize the compiler. If we have Interfaces.pickle, it
> produces a dependency:
> 
> - any IDL file => Interfaces.pickle
> - Interfaces.pickle => all generated C++ files
> 
> This implies that we have to rebuild all C++ files when any IDL file is
changed.
> 
> What's your plan about this?

That's a valid concern, but it actually won't happen.

Interfaces.pickle only includes very limited global information, like the
current
InterfaceDependencies.txt, so it very rarely changes (we use
--write_file_only_if_changed):
most IDL file changes don't change this file, and don't cause a rebuild.
(Indeed, it currently has exactly the same information as
InterfaceDependencies.txt,
which has the same issue, and I'm not going to add much.)

In fact, if you change [ImplementedAs=Foo] to [ImplementedAs=Bar] in X.idl,
all IDL files that use X need to be rebuilt, but this doesn't currently happen.
This is a bug (in current dependency resolution), but isn't a problem in
practice
b/c this is very rare (renaming C++ class).

This "global information file" is actually a simple way to *avoid* unnecessary
rebuilds:
any file that uses type X *actually* depends on X.idl (b/c [ImplementedAs] could
change),
but we don't want other changes in X.idl to cause rebuilds of everything using
type X!

We could do v. precise dependency computation, making an "external information"
file for each
IDL file (containing just the [ImplementedAs] and is_callback information), and
depend on *that*
file if X is used. I'll investigate this when doing the dependency rewrite, but
I suspect it's
lots of complexity for little gain.

Anyway, Interfaces.pickle won't cause frequent rebuilds, because it rarely
changes.

Does this make sense?

[haraken, 2014-01-06 08:15:29]

On 2014/01/06 06:38:46, Nils Barth wrote:
> On 2014/01/06 06:16:49, haraken wrote:
> > I'm a bit afraid that adding the intermediate file (Interfaces.pickle) might
> > make it harder to optimize the compiler. If we have Interfaces.pickle, it
> > produces a dependency:
> > 
> > - any IDL file => Interfaces.pickle
> > - Interfaces.pickle => all generated C++ files
> > 
> > This implies that we have to rebuild all C++ files when any IDL file is
> changed.
> > 
> > What's your plan about this?
> 
> That's a valid concern, but it actually won't happen.
> 
> Interfaces.pickle only includes very limited global information, like the
> current
> InterfaceDependencies.txt, so it very rarely changes (we use
> --write_file_only_if_changed):
> most IDL file changes don't change this file, and don't cause a rebuild.
> (Indeed, it currently has exactly the same information as
> InterfaceDependencies.txt,
> which has the same issue, and I'm not going to add much.)
> 
> In fact, if you change [ImplementedAs=Foo] to [ImplementedAs=Bar] in X.idl,
> all IDL files that use X need to be rebuilt, but this doesn't currently
happen.
> This is a bug (in current dependency resolution), but isn't a problem in
> practice
> b/c this is very rare (renaming C++ class).
> 
> This "global information file" is actually a simple way to *avoid* unnecessary
> rebuilds:
> any file that uses type X *actually* depends on X.idl (b/c [ImplementedAs]
could
> change),
> but we don't want other changes in X.idl to cause rebuilds of everything using
> type X!
> 
> We could do v. precise dependency computation, making an "external
information"
> file for each
> IDL file (containing just the [ImplementedAs] and is_callback information),
and
> depend on *that*
> file if X is used. I'll investigate this when doing the dependency rewrite,
but
> I suspect it's
> lots of complexity for little gain.
> 
> Anyway, Interfaces.pickle won't cause frequent rebuilds, because it rarely
> changes.
> 
> Does this make sense?

Help me understand:

Q1: Are you going to rely on write_file_only_if_changed?
write_file_only_if_changed works only in ninja. Is this OK for win build,
Android build etc?

Q2: If you rely on write_file_only_if_changed, it sounds reasonable to create
one file that describes global information. What global information are you
assuming? If you add parsing trees of all interfaces to the file, the file will
change every time.

[Nils Barth (inactive), 2014-01-06 10:21:35]

On 2014/01/06 08:15:29, haraken wrote:
> Help me understand:

n/p!

> Q1: Are you going to rely on write_file_only_if_changed?

Yes.
This is what we currently do - this CL doesn't change this.

compute_dependencies.py generates InterfaceDependencies.txt,
which is a dependency of all generated .cpp/.h files.
For non-ninja builds this is regenerated if any file is changed,
causing a full rebuild.
(Only ninja supports incremental builds.)

If we want to avoid full rebuilds on non-ninja builds, we'll need
substantial changes to dependency resolution.
I can do this (during dependency rewriting) if it's a priority.
(There's lots we can do to speed up and fix ninja build as well.)


> Q2: If you rely on write_file_only_if_changed, it sounds reasonable to create
> one file that describes global information.
> What global information are you assuming?

1. Dependency structure (dependent IDL files: partials and implements)
2. [ImplementedAs]
3. is_callback
4. list of ancestors

(1) is the data currently stored in InterfaceDependencies.txt.
I plan to add (2), (3), and (4) for Python compiler.


[ImplementedAs] and is_callback is global information, needed by any IDL file
that uses a type.
These are also very stable, so they should rarely change, hence ok to store
globally.

List of ancestors is basically dependency structure, but it's shallower:
(this is needed b/c e.g., IDL files that inherit from EventTarget have special
treatment)
it's separate b/c we don't need to depend on ancestor IDLs themselves.
E.g., if X.idl is a child of Y.idl and Y.idl is changed to inherit from
EventTarget,
we need to rebuild X, but we don't need to rebuild X if EventTarget.idl changes.

Ancestors should also be stable (rearranging the class hierarchy is rare and
probably warrants
a full rebuild anyway), but it's possible to simplify or eliminate this in
future
(e.g., per-file list of ancestors, or even an [Inherits=...] extended
attribute).


So global info is:
* Dependency structure, broadly (partials, implements, ancestors)
* External interface ([ImplementedAs], is_callback)
...which are both stable.


> If you add parsing trees of all interfaces to the file, the file will change
every time.

I don't plan to do this - as you note, we'd then just have one massive global
parse tree file!
If we want to store parse trees of IDL files (to avoid re-parsing), we'd do this
in individual files,
like X.tree or X.ast for X.idl, not in a global file.
(As you note, there are some benefits to this, so we'll investigate it during
dependency rewrite.)

[haraken, 2014-01-06 13:26:48]

> > Q1: Are you going to rely on write_file_only_if_changed?
> 
> Yes.
> This is what we currently do - this CL doesn't change this.
> 
> compute_dependencies.py generates InterfaceDependencies.txt,
> which is a dependency of all generated .cpp/.h files.
> For non-ninja builds this is regenerated if any file is changed,
> causing a full rebuild.
> (Only ninja supports incremental builds.)
> 
> If we want to avoid full rebuilds on non-ninja builds, we'll need
> substantial changes to dependency resolution.
> I can do this (during dependency rewriting) if it's a priority.
> (There's lots we can do to speed up and fix ninja build as well.)

The proposal in my speed-up document was avoiding full rebuilds on non-ninja
builds, but I agree that it might be over-engineering. I agree that we can
assume ninja.

> > Q2: If you rely on write_file_only_if_changed, it sounds reasonable to
create
> > one file that describes global information.
> > If you add parsing trees of all interfaces to the file, the file will change
> every time.
> 
> I don't plan to do this - as you note, we'd then just have one massive global
> parse tree file!
> If we want to store parse trees of IDL files (to avoid re-parsing), we'd do
this
> in individual files,
> like X.tree or X.ast for X.idl, not in a global file.
> (As you note, there are some benefits to this, so we'll investigate it during
> dependency rewrite.)

I don't fully understand how it looks like. If you generate individual ast
files, how can you specify the dynamic dependency from X.cpp to {X,Y,Z}.ast? If
you want to avoid the dynamism, you need to compile all the ast files into one
big file. Then you will end up having the original problem that the big file
changes every time so rebuild runs very frequently...

Either way, it looks like this CL is going to a right way in that it replaces
InterfaceDependency.txt with Interface.pickle which has a more general structure
than InterfaceDependency.txt. Thus LGTM.

https://codereview.chromium.org/124913002/diff/1/Source/bindings/derived_sour...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/124913002/diff/1/Source/bindings/derived_sour...
Source/bindings/derived_sources.gyp:165:
'<(SHARED_INTERMEDIATE_DIR)/blink/Interfaces.pickle',

Interfaces.pickle => InterfaceStructure.txt ?

https://codereview.chromium.org/124913002/diff/1/Source/bindings/derived_sour...
Source/bindings/derived_sources.gyp:166:
'<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt',

Not related to this CL, we discussed that we want to replace "derived sources"
with "generated code". I'd be happy if you could work on the clean-up when you
have a slot.

https://codereview.chromium.org/124913002/diff/1/Source/bindings/scripts/comp...
File Source/bindings/scripts/compute_dependencies.py (right):

https://codereview.chromium.org/124913002/diff/1/Source/bindings/scripts/comp...
Source/bindings/scripts/compute_dependencies.py:55:
parser.add_option('--interfaces-file', help='output pickle file')

--interface-structures-file ?

[Nils Barth (inactive), 2014-01-07 02:31:34]

On 2014/01/06 13:26:48, haraken wrote:
> The proposal in my speed-up document was avoiding full rebuilds on non-ninja
> builds, but I agree that it might be over-engineering. I agree that we can
> assume ninja.

This is an important distinction - thanks for clarifying (I'd been assuming
ninja implicitly).

Agreed, initially I'll assume ninja and optimize that,
as that's the main case and it should be simple;
after that I'll see how much work and complexity it is to optimize others.

Optimizing ninja is basically getting the dependencies computed right,
but optimizing non-ninja probably involves lots of GYP hacking for the other
build systems (>.<).

<snip: AST>
> I don't fully understand how it looks like. If you generate individual ast
> files, how can you specify the dynamic dependency from X.cpp to {X,Y,Z}.ast?
If
> you want to avoid the dynamism, you need to compile all the ast files into one
> big file. Then you will end up having the original problem that the big file
> changes every time so rebuild runs very frequently...

Good point - we can generate individual AST files that depend just on the IDL
file,
but .cpp files depend on multiple IDL files, so to avoid having to generate
dynamic dependencies (esp. for optimizing non-ninja),
we probably need to compute dependencies during regypping (i.e., have some
pymod_do_main)
so we have static dependencies, but then people would need to regyp all the
time,
which would defeat the point of having fast incremental rebuilds.
There might be a technique using generated .d files (as Nico suggested).
Anyway, if we generate AST files there's significant complexity to work out,
so I won't do it until I start on the dependency rewrite!


> Either way, it looks like this CL is going to a right way in that it replaces
> InterfaceDependency.txt with Interface.pickle which has a more general
structure
> than InterfaceDependency.txt. Thus LGTM.

Yup, that's exactly right - thanks!

[Nils Barth (inactive), 2014-01-07 02:41:03]

(Naming replies.)

https://codereview.chromium.org/124913002/diff/1/Source/bindings/derived_sour...
File Source/bindings/derived_sources.gyp (right):

https://codereview.chromium.org/124913002/diff/1/Source/bindings/derived_sour...
Source/bindings/derived_sources.gyp:165:
'<(SHARED_INTERMEDIATE_DIR)/blink/Interfaces.pickle',
On 2014/01/06 13:26:48, haraken wrote:
> Interfaces.pickle => InterfaceStructure.txt ?

"Interfaces.pickle" is a bit misleading
(it suggests it contains all the parsed interfaces).
(NB: It's a pickle, not txt)

How's "InterfacesInfo.pickle", since it's *about* interfaces?
(it's dependency structure, class names, etc.)

https://codereview.chromium.org/124913002/diff/1/Source/bindings/derived_sour...
Source/bindings/derived_sources.gyp:166:
'<(SHARED_INTERMEDIATE_DIR)/blink/BindingsDerivedSources.txt',
On 2014/01/06 13:26:48, haraken wrote:
> Not related to this CL, we discussed that we want to replace "derived sources"
> with "generated code". I'd be happy if you could work on the clean-up when you
> have a slot.

Good point - made a note of this.
Probably best to do when I start working on dependencies,
as then I'll be doing GYP hacking, so renaming makes a good preliminary CL.

[commit-bot: I haz the power, 2014-01-07 09:58:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/124913002/110001

[commit-bot: I haz the power, 2014-01-07 12:47:51]

Change committed as 164570