The cc/ directory is pretty big - is it time to break it up a bit?

[James Robinson]

The cc/ directory is currently rather large.  It has 384 C++ files, 176 of which are headers containing a little over 90k lines.  cc/test/ has another 70 source files with ~5k lines of code.  It's very convenient to have all of the compositor source in one directory, but I'm worried we are pushing the bounds of understandability.  Or possibly we crossed over it long ago and we just didn't notice.  Is it time to break this up?

I did a bit of analysis on C++ code in the rest of the Chromium repository (not stuff in third_party).  cc/ has the most source files in any single directory, just behind chrome/browser/extensions/ which has 432.  Just behind cc/ there's net/base/ which is 351 and base/ with 309.  In terms of headers, cc/ is in the lead followed by chrome/browser/extensions (165), net/base (146), base/ (132) and content/public/browser (129).  Here are some histograms (that may be buggy, I just hacked this code up quickly):

[0 - 8] 793 (56.7%)  .........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

[8 - 17] 254 (18.2%) (74.9%)  ..............................................................................................................................................................................................................................................................

[17 - 28] 127 (9.1%) (84.0%)  ...............................................................................................................................

[28 - 40] 73 (5.2%) (89.2%)  .........................................................................

[40 - 54] 54 (3.9%) (93.1%)  ......................................................

[54 - 70] 29 (2.1%) (95.1%)  .............................

[70 - 89] 21 (1.5%) (96.6%)  .....................

[89 - 110] 12 (0.9%) (97.5%)  ............

[110 - 135] 15 (1.1%) (98.6%)  ...............

[135 - 163] 7 (0.5%) (99.1%)  .......

[163 - 196] 7 (0.5%) (99.6%)  .......

[196 - 234] 2 (0.1%) (99.7%)  ..

[234 - 277] 0 (0.0%) (99.7%)  

[277 - 327] 1 (0.1%) (99.8%)  .

[327 - 384] 3 (0.2%) (100.0%)  ...

Headers

[0 - 3] 714 (51.1%)  ..........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

[3 - 7] 254 (18.2%) (69.2%)  ..............................................................................................................................................................................................................................................................

[7 - 12] 157 (11.2%) (80.5%)  .............................................................................................................................................................

[12 - 17] 81 (5.8%) (86.3%)  .................................................................................

[17 - 24] 76 (5.4%) (91.7%)  ............................................................................

[24 - 31] 40 (2.9%) (94.6%)  ........................................

[31 - 39] 21 (1.5%) (96.1%)  .....................

[39 - 49] 14 (1.0%) (97.1%)  ..............

[49 - 60] 15 (1.1%) (98.1%)  ...............

[60 - 72] 11 (0.8%) (98.9%)  ...........

[72 - 87] 5 (0.4%) (99.3%)  .....

[87 - 104] 5 (0.4%) (99.6%)  .....

[104 - 123] 0 (0.0%) (99.6%)  

[123 - 145] 2 (0.1%) (99.8%)  ..

[145 - 170] 3 (0.2%) (100.0%)  ...

cc/ is in the top bucket for both, of course, and it's relatively lonely.

I'm worried that the sheer number of files makes it harder to understand the compositor's code structure and organization and makes it harder to understand how it's supposed to be used as a library.  It's also harder to understand and enforce the logical dependencies inside cc/.  The downside of splitting things up would be it would be harder to find and edit files within the cc/ directory and moving files would be somewhat disruptive.  On balance, I currently think it'd be better to break things up than to not, but I could definitely be convinced otherwise.

Here's a strawman directory structure that divides things based on topic to see what it would look like.  I think this is an upper bound on the number of directories that would make sense.  I could definitely see collapsing some of these together, but we probably don't want more.  This is just a starting point for discussion and not at all set in stone:

cc/animation/ - *animation*, timing_function, transform_operations. 22 files

cc/base/ - common functionality with no dependencies on the rest of cc like thread, hash_pair, util, region, worker_pool, scoped_*. 27 files

cc/debug/ - support for heads_up_display, rendering_stats, devtools_instrumentation, debug colors, debug settings, frame_rate_counter, paint_time_counter. 17 files

cc/input/ - input_handler, top_controls_manager, page_scale_animation. 7 files

cc/layers/ - layer.h, *_layer.h, layer_impl.h, *_layer_impl.h and associated tests. 86 files

cc/scheduler/ - scheduler, scheduler_state_machine, time_sources, frame_rate_controller. 23 files

cc/quads/ - *_draw_quad.* and tests, and possibly the pass code as well. 24 files

cc/raster/ - rasterizing logic. all of our updaters, picture_pile_*, tiling managers, etc. 42 files

cc/renderer/ - renderer, direct_renderer, output_surface anything else that's not specific to GL vs software. 19 files

  cc/renderer/gl - gl-specific rendering stuff. gl_renderer, shader, program, geometry_bindings, texture_copier, mailbox stuff. 17 files

  cc/renderer/software - software-specific rendering stuff. software_renderer, _output_device, _frame_data. 7 files

cc/resources/ - resource, resource_pool, prioritized_resource(_manager)?, possibly priority_calculator, memory policy code. 32 files

cc/trees/ - stuff that deals with a layer tree. layer_tree_host(_impl|_common), tree_synchronizer, occlusion_tracker, proxies. 53 files

Here it is in spreadsheet form: https://docs.google.com/spreadsheet/ccc?key=0Ah3FsySZx1hSdDdZME9tUVU1X3F2QUp6NTY1bUlKa2c&usp=sharing.

Here would be the allowed dependencies:

Everything - can depend on cc/base (and probably cc/debug)

cc/animation - depends on nothing else

cc/debug - depends on nothing else

cc/input - depends on layers, trees

cc/layers - depends on animation, trees, quads, raster, resources

cc/scheduler - depends on nothing else

cc/quads - depends on resources

cc/raster - depends on layers, quads, resources

cc/renderer - depends on quads

cc/resources - depends on nothing else

cc/trees - depends on everything else

I like that this produces several leaf nodes in the tree and nothing is too huge, although "layers" and "trees" are both somewhat unwieldy.  I think we have a few cycles, and probably more than I'm thinking of here, but as long as it's controlled I think this is not the end of the world.  After all, cc is a single component.  I do think we want to maintain a few of these dependencies strictly, however.  For instance, quads shouldn't depend on layers no matter what.

Not listed here is cc/test/, which will continue to contain test infrastructure, depend on everything else in cc/ and not be depended on by anything else in cc/

What do you think?  Should we split things up at all or not?  If we do, how much splitting should we do?  The strawman would drop our average files per directory down to just under 30 which is definitely a big change.

- James

[Warren Hunt]

I'd strongly support breaking it up.  I did find the large number of files imposing as a noogler and I still find it annoying to navigate.

-Warren

[Vangelis Kokkevis]

I love the idea of breaking it up in more smaller bits. Thanks for taking a crack at this, James. My one comment on your proposal would be to try to keep the tree related files (both layers and layer tree hosts) together, and maybe breaking them up by thread instead (so all impl files in one spot and non-impl in another) to separate the bits that other users of this library may access directly (non-impl) from those that they're not supposed to touch (impl). 

Vangelis

On Fri, Mar 8, 2013 at 12:51 AM, James Robinson <jam...@chromium.org> wrote:

[Jonathan Dixon]

One thought: cc/renderer maybe confusing for outsides looking into cc/ as the three existing foo/renderer folders are for stuff only suitable for use in the renderer process.  If there's a synonym that doesn't start with 'r' that will be a tab completion win too :-)

[Slavomir Kaslev]

+1 Awesome! =-)

I like the general splitting scheme. And as the data suggests, cc/ is clearly an outlier.

How about moving some of the more generic utils to base/ insetad of cc/base/? In particular:

cc/hash_pair*

cc/scoped_ptr_*

cc/ring_buffer.h

cc/completion_event.h, cc/thread.h, cc/thread_impl.h

Thanks for bringing this up James.

--

Slavomir Kaslev | Software Engineer | ska...@google.com | 562 217 8497

[Shawn Singh]

+100

I'm not sure how practical it would be - would it work to have a cc/public/ directory, too, such that no code outside of cc/ should ever need to include anything outside of cc/public/ ?

[Darin Fisher]

As a general rule of thumb, we try not to move things to src/base pro-actively.  Otherwise, src/base would explode.  Instead, we use the rule that something belongs in src/base if it is in use by more than one module.  This is why we have src/{foo}/base/ directories.  Each module gets its own.

On Fri, Mar 8, 2013 at 11:26 AM, Slavomir Kaslev <ska...@google.com> wrote:

[Adrienne Walker]

Thanks for starting this discussion.  This proposed directory structure looks like a reasonable granularity to me.  I have a few quibbles (resource/raster split for some impl-side painting classes, but overall this looks great.

Even if the dependencies are circular between layers and trees, I still think it's valuable to separate them into different directory structures in the short term.  "What layer types are there?" is something that gets asked from time to time and it'd probably be helpful for new folks to have all those classes in the same directory.

I suspect that we could get layers to not depend on trees if we added a LayerClient interface and moved LayerTreeSettings out to base.

-enne

2013/3/8 James Robinson <jam...@chromium.org>

[Antoine Labour]

On Mon, Mar 11, 2013 at 9:24 AM, Adrienne Walker <en...@chromium.org> wrote:

Thanks for starting this discussion.  This proposed directory structure looks like a reasonable granularity to me.  I have a few quibbles (resource/raster split for some impl-side painting classes, but overall this looks great.

Even if the dependencies are circular between layers and trees, I still think it's valuable to separate them into different directory structures in the short term.  "What layer types are there?" is something that gets asked from time to time and it'd probably be helpful for new folks to have all those classes in the same directory.

I suspect that we could get layers to not depend on trees if we added a LayerClient interface and moved LayerTreeSettings out to base.

I don't think we should go out of our way to reduce circular dependencies between directories inside a component. I think it would only produce make work and slow us down. This is an area where I think we can keep ourselves honest through reviews.

Antoine

[Adrienne Walker]

2013/3/11 Antoine Labour <pi...@chromium.org>

On Mon, Mar 11, 2013 at 9:24 AM, Adrienne Walker <en...@chromium.org> wrote:

Even if the dependencies are circular between layers and trees, I still think it's valuable to separate them into different directory structures in the short term.  "What layer types are there?" is something that gets asked from time to time and it'd probably be helpful for new folks to have all those classes in the same directory.

I suspect that we could get layers to not depend on trees if we added a LayerClient interface and moved LayerTreeSettings out to base.

I don't think we should go out of our way to reduce circular dependencies between directories inside a component. I think it would only produce make work and slow us down. This is an area where I think we can keep ourselves honest through reviews

Absolutely agreed.  That last sentence was meant more to say "we could if we really wanted to," but I agree that review is probably sufficient to keep these dependencies in check.

-enne

[Nat Duca]

This sounds great.

No more client interfaces, pretty please. :)

[Vangelis Kokkevis]

Just to clarify, are we looking to split files thematically or based on intended usage? If we're trying to highlight bits that are necessary for using cc as a standalone compositor library, it seems to me that impl (and other files with code that's supposed to run on the thread) and non-impl files should be in different directories. 

On Mon, Mar 11, 2013 at 9:24 AM, Adrienne Walker <en...@chromium.org> wrote:

[James Robinson]

Thanks everyone for the feedback!  I'm glad that people think this is worth addressing.  The goal of these changes is to clarify and record our component's internal structure primarily for our own understanding (that is those of us who work actively on the compositor) but also for people trying to understand our component either to start contributing or to use it.  I think that many of these logical divisions already exist and reviewers will enforce them, but they aren't put into any sort of structure.  I'm really trying to record what we are already doing and make it easier to reason about pieces of the component and how they interact.

I've refined the proposal in response to the feedback here and in other places, you can see it at the same URL (https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0Ah3FsySZx1hSdDdZME9tUVU1X3F2QUp6NTY1bUlKa2c#gid=3) but under sheet 'Proposal 4'.  Navigate between sheets with the links at the bottom.  Keep in mind it'll probably be easier to divide things further if we need to than to split things up initially and then try to collapse.

Here's a summary of what's changed:

*) renderer/gl and renderer/software folded together to renderer.  They just aren't big enough as subdirectories or logical divisions to support a second nesting level.

*) raster/ and resources/ folded together into one directory, resources/.  Enne makes the good point that with impl-side painting the question of "how and what do I rasterize" and "how do I manage a resource budget" are really one and the same.  The file count for this directory will go down a good bit once we can remove the various foo_updater files

*) many misplaced files move into better homes

The stats for this breakdown:

10 directories (animation base debug input layers quads resources renderer scheduler trees)

Files per directory min/max/average: 7 (input) / 94 (layers) / 37.5

Here are things I haven't done and why:

After much discussion both in my head and with other folks I haven't added a public/ directory.  cc/ doesn't (and isn't intended to) support one set of stable APIs, it's used as a component in conjunction with other components within chromium.  Different embeddings need access to different portions of the component and the union of all access would be so large as to not be a useful distinction.  For instance, every embedding needs access to some set of cc::Layer types but only some need the cc::InputHandler abstractions and some (like the content/ embedding) need access to cc::RenderPass, all cc::DrawQuad subclasses, filters, etc.  Almost no other components in the chromium repo have a separate public/ directory except for content/ which is expected to be used from other repositories.

I also think it'd be a very bad idea to try to split things up based on thread usage.  Classes like cc::Layer and cc::LayerImpl have to remain tightly in sync with each other.  It's very rare to edit one and not the other.  We also have plenty of classes that span threads (i.e. cc::ThreadProxy) and trying to lump them into one group or the other would be arbitrary.  Even mostly-single-threaded classes like cc::Layer and cc::LayerTreeHost have very important functions that happen on the compositor thread.  Dealing with threading is a fact of life in cc/.

I also support making our primitives more generally available outside of cc if they are generally useful, but first we need to actually use the primitives already there in base/.  cc/thread.h for instance really doesn't have any reason to exist nowadays.  All the functionality it provides is already there in base's task runner abstractions.  Similarly cc/completion_event is a very simple wrapper around base::WaitableEvent that arguably doesn't deserve its own type (but gets it due to thread_restrictions).

- James

[Shawn Singh]

It looks like in the original proposal 1, layer_sorter was in the trees/ directory, which made sense to me.  What was the reason for moving it to layers/ ?  I like the idea of the layers/ directory being only about layer types and nothing else.  layerIterator I don't feel too strongly about, though.   But the sorter does make more sense to me as a part of layer tree functionality.  

[Dana Jansens]

On Tue, Mar 12, 2013 at 2:28 AM, Shawn Singh <shawn...@chromium.org> wrote:

It looks like in the original proposal 1, layer_sorter was in the trees/ directory, which made sense to me.  What was the reason for moving it to layers/ ?  I like the idea of the layers/ directory being only about layer types and nothing else.  layerIterator I don't feel too strongly about, though.   But the sorter does make more sense to me as a part of layer tree functionality.  

I would agree, it's something used in the LTHCommon code, it's only useful on a tree of layers. I think similarly of occlusion tracker (already in trees), layer iterator, quad sink, and render pass sink. These are all things that LTH uses, not that layers use.

[James Robinson]

On Tue, Mar 12, 2013 at 11:04 AM, Dana Jansens <dan...@chromium.org> wrote:

On Tue, Mar 12, 2013 at 2:28 AM, Shawn Singh <shawn...@chromium.org> wrote:

It looks like in the original proposal 1, layer_sorter was in the trees/ directory, which made sense to me.  What was the reason for moving it to layers/ ?  I like the idea of the layers/ directory being only about layer types and nothing else.  layerIterator I don't feel too strongly about, though.   But the sorter does make more sense to me as a part of layer tree functionality.  

I would agree, it's something used in the LTHCommon code, it's only useful on a tree of layers. I think similarly of occlusion tracker (already in trees), layer iterator, quad sink, and render pass sink. These are all things that LTH uses, not that layers use.

Fixed - it's back in trees/.  I've also renamed 'renderer' to 'output' since it's about producing some sort of output (pixels or serialized quads or whatnot) from a pass/quad list.  This also means it doesn't collide with content/renderer/ and everything under cc/ can be tab-completed in one char.

It seems like at least rough consensus has been reached, so I'll start moving things starting with the base/ stuff (which happens to churn the least) and moving my way directory by directory.

- James

[James Robinson]

First patch is up.  Star yourself to https://code.google.com/p/chromium/issues/detail?id=190824 to follow the rest.

[James Robinson]

The moves are complete as of r188707.  Since I've moved just under 500 files, it's pretty much a sure bet that I've got at least a few in the wrong location.  If you find something in the wrong place, just drop me a line or (even better) send a patch to fix it.

To update your local work, just git rebase.  git is amazingly good at dealing with this sort of thing.  You're most likely to get conflicts in #includes.  There are some useful scripts for dealing with this in tools:

tools/sort-headers.py can resort includes in a given file.  Run this after sorting out any #include conflicts to get things back into a good state.

tools/git/move_source_file.py can move a file to a new location, update include guards, update #includes and resort and update gyp files.  If you have any new files in a local branch, or want to fix the location of an existing file, this script handles very close to everything.  The one thing to watch out for with this script is sometimes it'll update unrelated gyp files if the partial path matches.  I accidentally busterated the tree with these even after writing out this reminder so be careful.

Going forward, new code should go into one of the existing directories or (if it doesn't fit) into a new directory.

- James

[James Robinson]

One more note about history:  The file moves are all moves+edits in the SVN repo, so things like annotate and git blame will do the right thing. To view the history of a specific file past the move, pass the "--follow" flag to git log as in:

git log --follow cc/output/program_binding.cc

- James

[Vangelis Kokkevis]

Woot!! Huge thanks, James!

Vangelis

[Thiago Farina]

On Fri, Mar 8, 2013 at 5:51 AM, James Robinson <jam...@chromium.org> wrote:
> The cc/ directory is currently rather large. It has 384 C++ files, 176 of
> which are headers containing a little over 90k lines. cc/test/ has another
> 70 source files with ~5k lines of code. It's very convenient to have all of
> the compositor source in one directory, but I'm worried we are pushing the
> bounds of understandability. Or possibly we crossed over it long ago and we
> just didn't notice. Is it time to break this up?
>

> Here it is in spreadsheet form:
> https://docs.google.com/spreadsheet/ccc?key=0Ah3FsySZx1hSdDdZME9tUVU1X3F2QUp6NTY1bUlKa2c&usp=sharing.
>
> Here would be the allowed dependencies:
>
> Everything - can depend on cc/base (and probably cc/debug)
>
> cc/animation - depends on nothing else
>

Is it worth using DEPS file to enforce this?

> cc/debug - depends on nothing else
>
> cc/input - depends on layers, trees
>
> cc/layers - depends on animation, trees, quads, raster, resources
>
> cc/scheduler - depends on nothing else
>
> cc/quads - depends on resources
>
> cc/raster - depends on layers, quads, resources
>
> cc/renderer - depends on quads
>
> cc/resources - depends on nothing else
>
> cc/trees - depends on everything else
>

--
Thiago

[James Robinson]

On Thu, Jul 11, 2013 at 8:18 PM, Thiago Farina <tfa...@chromium.org> wrote:

On Fri, Mar 8, 2013 at 5:51 AM, James Robinson <jam...@chromium.org> wrote:

> Here would be the allowed dependencies:
>
> Everything - can depend on cc/base (and probably cc/debug)
>
> cc/animation - depends on nothing else
>

Is it worth using DEPS file to enforce this?

That's a good question.  It's a judgement call - there is probably no truly correct answer. When doing this split, I constructed the full matrix of what-should-depend-on-what and a few of us cc/ OWNERS discussed whether to use DEPS or not.  The conclusion at the time was it wasn't worth doing this since cc/ is a single component with a single set of OWNERS so the normal review process should be sufficient to maintain the internal structure of the component.  Using DEPS for everything seemed overly prescriptive.

I don't think we have had many issues with people introducing bad dependencies since then, so I think the same logic applies today.  That said cc/ is a bit bigger now than it was 5 months ago and people other than me are doing most of the reviews in cc/, so I wouldn't mind if the other OWNERS feel this would help.

- James

[Adrienne Walker]

I agree with James that DEPS in cc would cause more pain than they
would be worth.

2013/7/11 James Robinson <jam...@chromium.org>: