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

Steel thread for new lookup code #2622

Merged
merged 13 commits into from Apr 27, 2021
Merged

Conversation

jcollins-g
Copy link
Contributor

@jcollins-g jcollins-g commented Apr 22, 2021

This implements (but does not activate by default) a new comment reference resolution system for dartdoc that can help us migrate off the heuristic trickery currently in markdown_processor.dart and finish the extension method implementation.

The new one is known to have unimpressive accuracy for now, but should improve with further commits.

An example with the self-checking code turned on to run both kinds of resolution and compare their results:

jcollins-macbookpro2:dartdoc jcollins$ dart bin/dartdoc.dart --sdk-docs --warnings reference-lookup-found-with-new,reference-lookup-missing-with-new,reference-lookup-not-found-with-new --output /tmp/adirsomewhere --show-stats
[...]
  warning: reference lookup found with new: [E] => element: E warn: true
    from dart-js.JsArray.length: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/js/js.dart:189:20)
    from documentation for symbol dart-core.List.length=: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/core/list.dart:308:7)
[...]
 warning: reference lookup resolution differs between lookup implementations:  [Float64List] => new: element: dart:typed_data.Float64List.Float64List warn: true old: element: dart:typed_data.Float64List warn: true, from dart-typed_data.Float64List.Float64List: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/typed_data/typed_data.dart:1913:20)
  warning: reference lookup resolution differs between lookup implementations:  [Float64List] => new: element: dart:typed_data.Float64List.Float64List warn: true old: element: dart:typed_data.Float64List warn: true, from dart-typed_data.Float64List.fromList: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/typed_data/typed_data.dart:1920:32)
  warning: reference lookup resolution differs between lookup implementations:  [Float64List] => new: element: dart:typed_data.Float64List.Float64List warn: true old: element: dart:typed_data.Float64List warn: true, from dart-typed_data.Float64List.sublistView: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/typed_data/typed_data.dart:1978:23)
  warning: reference lookup found only in old lookup code: [List.length] => element: dart:core.List.length warn: true, from dart-typed_data.Float64List.sublistView: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/typed_data/typed_data.dart:1978:23)
[...]
Generating docs for library dart:web_sql from dart:web_sql...
  warning: unresolved doc reference [callback], from dart-web_sql.SqlDatabase.changeVersion: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/web_sql/dart2js/web_sql_dart2js.dart:119:26)
  warning: unresolved doc reference [successCallback], from dart-web_sql.SqlDatabase.changeVersion: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/web_sql/dart2js/web_sql_dart2js.dart:119:26)
  warning: unresolved doc reference [errorCallback], from dart-web_sql.SqlDatabase.changeVersion: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/web_sql/dart2js/web_sql_dart2js.dart:119:26)
  warning: reference lookup found only in old lookup code: [StateError] => element: dart:core.StateError warn: true
    from dart-web_sql.SqlResultSetRowList.first: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/web_sql/dart2js/web_sql_dart2js.dart:240:11)
    from documentation for symbol dart-core.Iterable.first: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/core/iterable.dart:465:9)
  warning: reference lookup found only in old lookup code: [StateError] => element: dart:core.StateError warn: true
    from dart-web_sql.SqlResultSetRowList.last: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/web_sql/dart2js/web_sql_dart2js.dart:247:11)
    from documentation for symbol dart-core.Iterable.last: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/core/iterable.dart:481:9)
  warning: reference lookup found only in old lookup code: [StateError] => element: dart:core.StateError warn: true
    from dart-web_sql.SqlResultSetRowList.single: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/web_sql/dart2js/web_sql_dart2js.dart:255:11)
    from documentation for symbol dart-core.Iterable.single: (file:///Users/jcollins/dart/all_sdks/2.14.0-21.0.dev/lib/core/iterable.dart:496:9)
Validating docs...
  warning: dartdoc generated a broken link to: dart-math
Found 2115 warnings and 0 errors.
Documented 19 public libraries in 98.1 seconds
Reference Counts:
total references: 29575
resolved references:  22321 (75.47252747252747%)
resolved references with new lookup:  20670 (69.89010989010988%)
resolved references with old lookup:  22321 (75.47252747252747%)
resolved references with equivalent links:  19994 (67.6043956043956%)

The new code produces equivalent results to the old one 68% of the time on the SDK, meaning that if we switched this code on today we'd have a different results for about a third of the comment references in the SDK (presumably, wrongly). We can iterate down on this by checking the generated warnings and removing differences when appropriate, adding tests for them, and ensuring that extension methods work properly with new tests.

@google-cla google-cla bot added the cla: yes Google CLA check succeeded. label Apr 22, 2021
@srawlins
Copy link
Member

Small PR; should be a quick review 😛

Seriously though, I'll prioritize reviewing this.

@coveralls
Copy link

coveralls commented Apr 22, 2021

Coverage Status

Coverage decreased (-1.4%) to 57.473% when pulling 95231cb on jcollins-g:lookup into 6446a92 on dart-lang:master.

@jcollins-g
Copy link
Contributor Author

I'm hoping the review won't be too bad; a good portion of the line count is boilerplate and generated file changes.

Copy link
Member

@srawlins srawlins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 27/34 files so far. Thought I'd mail out what I have so far.

lib/src/markdown_processor.dart Outdated Show resolved Hide resolved
lib/src/markdown_processor.dart Show resolved Hide resolved
lib/src/markdown_processor.dart Outdated Show resolved Hide resolved
lib/src/markdown_processor.dart Show resolved Hide resolved
lib/src/markdown_processor.dart Outdated Show resolved Hide resolved
lib/src/markdown_processor.dart Show resolved Hide resolved
lib/src/model/extension.dart Outdated Show resolved Hide resolved
lib/src/model/getter_setter_combo.dart Show resolved Hide resolved
lib/src/model/library.dart Outdated Show resolved Hide resolved
Copy link
Member

@srawlins srawlins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very excited.

@@ -0,0 +1,81 @@
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2021?

lib/src/model/comment_referable.dart Outdated Show resolved Hide resolved
lib/src/model/comment_referable.dart Outdated Show resolved Hide resolved
lib/src/model/comment_referable.dart Outdated Show resolved Hide resolved
lib/src/model/comment_referable.dart Outdated Show resolved Hide resolved
lib/src/model/comment_referable.dart Outdated Show resolved Hide resolved
lib/src/model/comment_referable.dart Outdated Show resolved Hide resolved
test/comment_referable_test.dart Outdated Show resolved Hide resolved
test/comment_referable_test.dart Outdated Show resolved Hide resolved
test/comment_referable_test.dart Outdated Show resolved Hide resolved
@srawlins
Copy link
Member

Have you done a benchmark of the experimental resolution vs the current resolution? Seems like replacing the algorithm could really affect runtime and also peak memory.

@jcollins-g
Copy link
Contributor Author

@srawlins some benchmark results:

SDK is 1375M peak memory in new code vs 1509M. I expect this to improve with further commits; it's likely we're still hanging onto or generating some tables we no longer need. Documentation generation stage for the SDK is 93.4 seconds vs 101.9 seconds in the old code. I can't fully predict yet whether this will improve or degrade as the implementation improves -- I'm hoping that when we are no longer generating the detailed warning messages for references we can't resolve currently and when we clean up the table generation we'll gain some back, but there are probably some cases we have to implement that could slow us down.

@jcollins-g jcollins-g merged commit f9a875d into dart-lang:master Apr 27, 2021
@jcollins-g jcollins-g deleted the lookup branch April 27, 2021 21:19
@srawlins
Copy link
Member

Whoa those benchmark results are really promising! I mean, this new impl wasn't intended to improve performance, and generally speaking, improved algorithms come with more memory and/or speed. It's great even if this work keeps them on par, but improving one or both would be stellar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants