Skip to content

Fix loading performance for pages with many links caused by excessive re-computation #900

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

Merged
merged 9 commits into from
Oct 9, 2024

Conversation

mportiz08
Copy link
Contributor

Bug/issue #, if applicable: 136247329 (#894)

Summary

This fixes a performance regression (#894) when loading pages with many links that was introduced with #878.

The problem is that any logic in the referencesProvider mixin does not get cached once for each client as I mistakenly assumed, so every component using this mixin was unintentionally re-running the logic for inactivating known external links, which becomes noticeable when there is a big list of references.

To fix it, I moved this logic to the setReferences methods in the appropriate stores, and I'm making sure to update the refs and re-run this logic whenever the global includedArchiveIdentifiers data changes.

(I closed #898 which was a less ideal solution to this same issue—thanks for pointing out the root issue that I missed @hqhhuang!)

Testing

Describe how a reviewer can test the functionality of your PR. Provide test content to test with if
applicable.

Steps:

  1. Follow and verify the testing instructions for the original PR in Inactivate known external links #878.
  2. Verify that the performance issue from Slow rendering of root-level pages #894 is now also resolved.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran npm test, and it succeeded
  • Updated documentation if necessary

@mportiz08 mportiz08 linked an issue Oct 8, 2024 that may be closed by this pull request
2 tasks
@mportiz08
Copy link
Contributor Author

@swift-ci test

@mportiz08
Copy link
Contributor Author

@hqhhuang I removed the update logic from non-documentation-page components as we discussed offline since it's not technically necessary there yet.

@mportiz08
Copy link
Contributor Author

@swift-ci test

1 similar comment
@mportiz08
Copy link
Contributor Author

@swift-ci test

@mportiz08 mportiz08 merged commit f7c7899 into swiftlang:main Oct 9, 2024
1 check passed
@mportiz08 mportiz08 deleted the avoid-recomputing-refs-in-mixin branch October 9, 2024 18:46
@heckj
Copy link
Member

heckj commented Nov 1, 2024

Is this PR included in the content that's exposed with swift 6.0.2 (toolchain with Xcode 16.1), or is it coming in a future version of the swift toolchain (6.1?) (was it cherry picked?)

@mportiz08
Copy link
Contributor Author

Is this PR included in the content that's exposed with swift 6.0.2 (toolchain with Xcode 16.1), or is it coming in a future version of the swift toolchain (6.1?) (was it cherry picked?)

I believe it is only on the latest development Swift toolchain at the moment.

If you need a workaround using an existing toolchain, you can clone the latest version of swift-docc-render-artifact and set DOCC_HTML_DIR=/path/to/swift-docc-render-artifact/dist as part of a docc command invocation to use the latest version of the renderer that includes this fix.

mportiz08 added a commit to mportiz08/swift-docc-render that referenced this pull request Nov 4, 2024
mportiz08 added a commit that referenced this pull request Nov 4, 2024
anferbui pushed a commit to anferbui/swift-docc-render that referenced this pull request Nov 7, 2024
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.

Slow rendering of root-level pages
3 participants