Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More detailed logging and APIChanges refactoring #941

Merged
merged 2 commits into from
Nov 11, 2013

Conversation

gkossakowski
Copy link
Contributor

The 7bc5aac commit contains some more detailed logging. See the commit message for details.

The main motivation behind this PR is to reify information about api changes that incremental compiler considers. We introduce a new sealed class APIChange that has (at the moment) two subtypes:

  • APIChangeDueToMacroDefinition - as the name explains, this represents the case where incremental compiler considers an api to be changed just because given source file contains a macro definition
  • SourceAPIChange - this represents the case of regular api change; at the moment it's just a simple wrapper around value representing source file but in the future it will get expanded to contain more detailed information about API changes (e.g. collection of changed name hashes)

The APIChanges becomes just a collection of APIChange instances. In particular, I removed names field that seems to be a dead code in incremental compiler. The NameChanges class and methods that refer to
it in SameAPI has been deprecated.

The Incremental.scala has been adapted to changed signature of APIChanges class. The sameSource method returns representation of APIChange (if there's one) instead of just simple boolean. One notable change is
that information about APIChanges is pushed deeper into invalidation logic. This will allow us to treat the APIChangeDueToMacroDefinition case properly once name hashing scheme arrives.

This commit shouldn't change any behavior and is purely a refactoring.

@harrah
Copy link
Member

harrah commented Oct 29, 2013

What does typical debug output look like with these changes? It seems like there will be a lot of information dumped for a reasonably sized project and what to log should instead be refined by one or more switches.

@gkossakowski
Copy link
Contributor Author

I just tested the worst case: Scala compiler that has super deep inheritance hierarchies and all 290 files get invalidated. Here are the results: https://gist.github.com/gkossakowski/7222218 (there two files there)

The total log grows from 92kb to 250kb. It's significant change but I think it's still within margin of what we can log by default. I added this logging because I found it crucial for analyzing incremental's compiler behavior.

We should remember that recompiling the whole project (290 files) and super deep hierarchies is not what we usually expect.

@harrah
Copy link
Member

harrah commented Nov 4, 2013

Thanks. I'm not worried about the size, but the ability to get useful information out. The gist makes it look like the new logging dominates all of the other information. Is this the case on the terminal as well or is it because of line wrapping differences? Does it make more sense to group the output by the invaliding file?

(The overall idea behind the pull request is fine, just refining...)

@gkossakowski
Copy link
Contributor Author

If the size/performance is not a concern then let me prepare an output of more regular case than Scala compiler.

The following events are logged:

  * invalidation of source file due to macro definition
  * inclusion of dependency invalidated by inheritance; we log both
    nodes of dependency edge (dependent and dependency)

The second bullet helps to understand what's going on in case of
complex inheritance hierarchies like in Scala compiler.
The main motivation behind this commit is to reify information about
api changes that incremental compiler considers. We introduce a new
sealed class `APIChange` that has (at the moment) two subtypes:

  * APIChangeDueToMacroDefinition - as the name explains, this represents
    the case where incremental compiler considers an api to be changed
    just because given source file contains a macro definition
  * SourceAPIChange - this represents the case of regular api change;
    at the moment it's just a simple wrapper around value representing
    source file but in the future it will get expanded to contain more
    detailed information about API changes (e.g. collection of changed
    name hashes)

The APIChanges becomes just a collection of APIChange instances.
In particular, I removed `names` field that seems to be a dead code in
incremental compiler. The `NameChanges` class and methods that refer to
it in `SameAPI` has been deprecated.

The Incremental.scala has been adapted to changed signature of APIChanges
class. The `sameSource` method returns representation of APIChange
(if there's one) instead of just simple boolean. One notable change is
that information about APIChanges is pushed deeper into invalidation logic.
This will allow us to treat the APIChangeDueToMacroDefinition case properly
once name hashing scheme arrives.

This commit shouldn't change any behavior and is purely a refactoring.
@gkossakowski
Copy link
Contributor Author

I talked to @harrah and we decided it can go in.

When it comes to grouping by file that causes invalidation, that will come later on. I have a patch coming which refactors the invalidation logic to perform invalidation for each initial source separately. Once that patch is in, logging per file will become trivial.

gkossakowski added a commit that referenced this pull request Nov 11, 2013
More detailed logging and APIChanges refactoring
@gkossakowski gkossakowski merged commit f2bdeff into sbt:0.13 Nov 11, 2013
@gkossakowski gkossakowski deleted the apichanges-refactor branch November 11, 2013 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants