Skip to content

Per-request caches and dependency maps #35206

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 6 commits into from
Jan 6, 2021

Conversation

slavapestov
Copy link
Contributor

Based on @hamishknight's PR #31143.

@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 7b86366d01870ed21f55e980dfa1f6428708b57c

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 7b86366d01870ed21f55e980dfa1f6428708b57c

@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 2ffe1d3d8dc37581a6cba8acf6832d95d57e94e4

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 2ffe1d3d8dc37581a6cba8acf6832d95d57e94e4

@slavapestov
Copy link
Contributor Author

swiftlang/llvm-project#2279
@swift-ci Please test

@hamishknight
Copy link
Contributor

Glad you were able to put the patch to use! Another approach I was experimenting with (originally suggested by @CodaFi) was to instead allow small requests and outputs to be stored inline in AnyRequest & AnyValue, which would come with a couple of benefits:

  • It would allow large requests to have their storage shared by both the evaluator and dependency tracker
  • AnyValue would be able to keep its indirection in certain cases, which could allow for the evaluator returning certain outputs by reference, which could cut down on copies for e.g cached std::vector returns, and would allow for move-only cached outputs.

However I was getting some regressions for AnyRequest which I didn't get round to fully tracking down (applying the change just to AnyValue gave a decent performance win though). I'd still be interested in pursuing it further to see what the resulting win would be (if any), but am also more than happy to take the concrete performance win in this change :)

I was also planning on (but never started) experimenting with stamping out specialized caches for e.g AST nodes that would store all the request outputs together (essentially mimicking external caching storage but encapsulated within the evaluator), but that can be done on top of any caching implementation.

This becomes tricky with the new per-request cache representation.

We can add it back later if we need it, but I feel like this code
path isn't particularly useful right now anyway.
This is based on an earlier patch by @hamishknight.

The idea is that instead of caching results in a single DenseMap
that maps AnyRequest to AnyValue, we instead define a separate
DenseMap for each request kind that directly uses the request as
the key, and the request value as the value.

This avoids type erasure and memory allocation overhead arising
from the use of AnyRequest and AnyValue. There are no remaining
usages of AnyValue, and the only usage of AnyRequest is now in
the reference dependency tracking, which can be refactored to use
a similar strategy of storing per-request maps as well.
... instead of passing around an ActiveRequest.
Just as with the result cache, instead of a single DenseMap with
type-erased AnyRequest keys, we can use per-request maps for a
nice performance improvement.
@slavapestov slavapestov force-pushed the per-request-caches branch 2 times, most recently from 00ed02d to 0bfa868 Compare January 5, 2021 22:58
@slavapestov
Copy link
Contributor Author

@hamishknight I'll leave AnyValue and AnyRequest in place for now.

@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

swiftlang/llvm-project#2279
@swift-ci Please test

@slavapestov
Copy link
Contributor Author

swiftlang/llvm-project#2279
@swift-ci Please test source compatibility

@slavapestov slavapestov marked this pull request as ready for review January 5, 2021 23:30
@swift-ci
Copy link
Contributor

swift-ci commented Jan 6, 2021

Build failed
Swift Test Linux Platform
Git Sha - 0bfa868

@slavapestov
Copy link
Contributor Author

@swift-ci Please test Linux

@swift-ci
Copy link
Contributor

swift-ci commented Jan 6, 2021

Build failed
Swift Test Linux Platform
Git Sha - 0bfa868

@slavapestov
Copy link
Contributor Author

swiftlang/llvm-project#2279
@swift-ci Please test Linux

@slavapestov
Copy link
Contributor Author

swiftlang/llvm-project#2279
@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

swift-ci commented Jan 6, 2021

Build failed
Swift Test Linux Platform
Git Sha - 0bfa868

@slavapestov
Copy link
Contributor Author

swiftlang/llvm-project#2279
@swift-ci Please test Linux

@swift-ci
Copy link
Contributor

swift-ci commented Jan 6, 2021

Build failed
Swift Test Linux Platform
Git Sha - 0bfa868

@slavapestov
Copy link
Contributor Author

@swift-ci Please test Linux

@slavapestov slavapestov merged commit be8f4cc into swiftlang:main Jan 6, 2021
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.

3 participants