Skip to content

Conversation

@franklinsch
Copy link
Contributor

@franklinsch franklinsch commented Feb 7, 2022

Bug/issue #, if applicable: rdar://88464797

Summary

Articles that document a module available in multiple languages now inherit that module's languages. This ensures that we generate multiple language variants for article pages.

While investigating the performance impact of this change, I found that the repeated accesses to the DocumentationContext.rootModules (a computed property that iterates over all topic graph nodes) was causing a large performance regression. To resolve this, in the second commit I made the property a stored property instead that's initialised once during bundle registration, right after symbols have been loaded in the symbol graph.

Performance impact

No statistically significant impact.

Size 10
+-----------------------------------------------------------------------------------------------------+
| Metric                                   | Change     | Before               | After                |
+-----------------------------------------------------------------------------------------------------+
| Compiled output size (MB)                | no change  | 114.68               | 114.68               |
| Duration for 'bundle-registration' (sec) | +1.58%     | 5.76                 | 5.85                 |
| Duration for 'convert-action' (sec)      | +2.93%     | 7.78                 | 8.00                 |
| Peak memory footprint (MB)               | -1.18%     | 354.33               | 350.14               |
| Topic Anchor Checksum                    | no change  | c8f03d4e81da8e3b6d7f | c8f03d4e81da8e3b6d7f |
| Topic Graph Checksum                     | no change  | edce22bce4f8de475a5e | edce22bce4f8de475a5e |
+-----------------------------------------------------------------------------------------------------+

Size 25
+-----------------------------------------------------------------------------------------------------+
| Metric                                   | Change     | Before               | After                |
+-----------------------------------------------------------------------------------------------------+
| Compiled output size (MB)                | no change  | 287.62               | 287.62               |
| Duration for 'bundle-registration' (sec) | +0.26%     | 14.75                | 14.79                |
| Duration for 'convert-action' (sec)      | -0.03%     | 20.09                | 20.08                |
| Peak memory footprint (MB)               | -0.68%     | 816.43               | 810.91               |
| Topic Anchor Checksum                    | no change  | 9a675b9ad6d69f8b7f0c | 9a675b9ad6d69f8b7f0c |
| Topic Graph Checksum                     | no change  | 665713509084b5131a37 | 665713509084b5131a37 |
+-----------------------------------------------------------------------------------------------------+

Size 5
+-----------------------------------------------------------------------------------------------------+
| Metric                                   | Change     | Before               | After                |
+-----------------------------------------------------------------------------------------------------+
| Compiled output size (MB)                | no change  | 57.36                | 57.36                |
| Duration for 'bundle-registration' (sec) | -0.31%     | 2.94                 | 2.93                 |
| Duration for 'convert-action' (sec)      | -0.51%     | 3.95                 | 3.93                 |
| Peak memory footprint (MB)               | -5.21%     | 209.86               | 198.93               |
| Topic Anchor Checksum                    | no change  | 65d4ff3050cd1106a21b | 65d4ff3050cd1106a21b |
| Topic Graph Checksum                     | no change  | 0a64c740a2ed5a246836 | 0a64c740a2ed5a246836 |
+-----------------------------------------------------------------------------------------------------+

Size 50
+-----------------------------------------------------------------------------------------------------+
| Metric                                   | Change     | Before               | After                |
+-----------------------------------------------------------------------------------------------------+
| Compiled output size (MB)                | no change  | 578.63               | 578.63               |
| Duration for 'bundle-registration' (sec) | -1.07%     | 30.17                | 29.84                |
| Duration for 'convert-action' (sec)      | +0.47%     | 41.06                | 41.25                |
| Peak memory footprint (MB)               | -0.90%     | 1552.76              | 1538.81              |
| Topic Anchor Checksum                    | no change  | 08d0a84bec913460905f | 08d0a84bec913460905f |
| Topic Graph Checksum                     | no change  | 74d25c4f1ee185ad5676 | 74d25c4f1ee185ad5676 |
+-----------------------------------------------------------------------------------------------------+

Dependencies

None.

Testing

When documenting a framework that supports Swift and Objective-C, verify that articles, whether they're automatically or manually curated, get the language switcher in the sidebar. The syntax used in symbol references should reflect the currently selected language.

TODO

  • Investigate performance impact

Checklist

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

@franklinsch franklinsch force-pushed the article-language-variants branch 2 times, most recently from 8cb3303 to c10c1d8 Compare February 7, 2022 15:02
@franklinsch
Copy link
Contributor Author

@swift-ci please test

@franklinsch franklinsch force-pushed the article-language-variants branch from c10c1d8 to a3de982 Compare February 7, 2022 17:00
@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Feb 8, 2022

Also I suggest changing the "name" and "id" definitions in SourceLanguage.swift from "var" to "let" by the way.

https://github.com/apple/swift-docc/blob/779ca826b7902880477a3bfcc0078970284fb505/Sources/SwiftDocC/Model/SourceLanguage.swift#L12-L16

@franklinsch franklinsch force-pushed the article-language-variants branch from a3de982 to c1ce32f Compare February 8, 2022 12:09
@franklinsch
Copy link
Contributor Author

@swift-ci test

Copy link
Contributor

@ethan-kusters ethan-kusters left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for putting this together.

I remember when initially implementing #47 that there was some remaining work to support auto-curated symbol collections (for protocol inherited symbols and the like) in multi-language scenarios since those rely on articles. Is that something we can resolve in this PR? (Or are we already and I missed it?)


// If available source languages are provided and it contains Swift, use Swift as the default language of
// the article.
let sourceLanguage = availableSourceLanguages.map { availableSourceLanguages in
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
let sourceLanguage = availableSourceLanguages.map { availableSourceLanguages in
let defaultSourceLanguage = availableSourceLanguages.map { availableSourceLanguages in

// curate articles.
rootModules = topicGraph.nodes.values.compactMap { node in
guard node.kind == .module,
!onlyHasSnippetRelatedChildren(for: node.reference) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

This snippet check is a little confusing to me. Maybe we should consider a node.isSnippetModule property at some point?

It almost seems like we should have a separate kind .snippetModule if we're going to need to special-case these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. This logic was just moved in this PR, but we should consider revisiting this in the future.

Articles that document a module available in multiple languages now
inherit that module's languages. This ensures that we generate multiple
language variants for article pages.

rdar://88464797
Makes DocumentationContext.rootModules a stored property instead of a
computed property for performance reasons.
@franklinsch franklinsch force-pushed the article-language-variants branch from 5694c1f to 70b1352 Compare February 11, 2022 09:45
@franklinsch
Copy link
Contributor Author

I remember when initially implementing #47 that there was some remaining work to support auto-curated symbol collections (for protocol inherited symbols and the like) in multi-language scenarios since those rely on articles. Is that something we can resolve in this PR? (Or are we already and I missed it?)

I don't think that would be affected by these changes. This PR should only affect user-authored articles. Do we have a bug to track the issue you're referring to?

@ethan-kusters
Copy link
Contributor

@swift-ci please test

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.

4 participants