eskipfile: optimize Watch.LoadUpdates #3448
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AlexanderYastrebov
force-pushed
the
eskipfile/benchmark-watch-load-update
branch
2 times, most recently
from
ac1126a to
69b94cc
Compare
AlexanderYastrebov
force-pushed
the
eskipfile/benchmark-watch-load-update
branch
from
69b94cc to
c308d19
Compare
|
👍 |
szuecs
reviewed
Mar 25, 2025
| quit chan struct{} | ||
| once sync.Once | ||
| fileName string | ||
| lastContent []byte |
There was a problem hiding this comment.
Maybe we should better store the sha256 value and compare that one to detect a change, because it is really fast and doesn't require to duplicate the memory of routes as byte slice.
There was a problem hiding this comment.
Its fast but not computing it is even faster :)
Do we care about memory consumed by eskip given that we parse and keep routes for lookup anyways?
There was a problem hiding this comment.
I think we can move forward with this, given that the concern with large eskipfiles is more about network not memory, and we can update later if we need.
Signed-off-by: Alexander Yastrebov <[email protected]>
Do not parse routes if file content did not change since last successful read. Note that after #2304 RemoteWatch does not update file if response Etag did not change but underlying Watch would still parse routes. ``` goos: linux goarch: amd64 pkg: github.com/zalando/skipper/eskipfile cpu: Intel(R) Core(TM) i5-8350U CPU @ 1.70GHz │ HEAD~1 │ HEAD │ │ sec/op │ sec/op vs base │ WatchLoadUpdate-8 17889.7µ ± 24% 220.0µ ± 24% -98.77% (p=0.000 n=10) │ HEAD~1 │ HEAD │ │ B/op │ B/op vs base │ WatchLoadUpdate-8 7837.4Ki ± 0% 528.5Ki ± 0% -93.26% (p=0.000 n=10) │ HEAD~1 │ HEAD │ │ allocs/op │ allocs/op vs base │ WatchLoadUpdate-8 110110.000 ± 0% 6.000 ± 0% -99.99% (p=0.000 n=10) ``` Signed-off-by: Alexander Yastrebov <[email protected]>
AlexanderYastrebov
force-pushed
the
eskipfile/benchmark-watch-load-update
branch
from
c308d19 to
f78437d
Compare
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Do not parse routes if file content did not change since last successful read.
Note that after #2304 RemoteWatch does not update file if response Etag did not change but underlying Watch would still parse routes.