-
Notifications
You must be signed in to change notification settings - Fork 2
Cache tool #5
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
Cache tool #5
Conversation
25a3572 to
570bbdf
Compare
cmd/traffic_cache_tool/CacheTool.cc
Outdated
| using ts::MemView; | ||
| using ts::CacheDirEntry; | ||
|
|
||
| #define VOL_HASH_TABLE_SIZE 32707 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No #define unless absolutely necessary. constexpr.
cmd/traffic_cache_tool/CacheTool.cc
Outdated
| using ts::CacheDirEntry; | ||
|
|
||
| #define VOL_HASH_TABLE_SIZE 32707 | ||
| #define VOL_HASH_ALLOC_SIZE (8 * 1024 * 1024) // one chance per this unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a Scalar instance.
cmd/traffic_cache_tool/CacheTool.cc
Outdated
| #define VOL_HASH_TABLE_SIZE 32707 | ||
| #define VOL_HASH_ALLOC_SIZE (8 * 1024 * 1024) // one chance per this unit | ||
| #define VOL_HASH_EMPTY 0xFFFF | ||
| #define STORE_BLOCK_SHIFT 13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed, handled by Scalar.
cmd/traffic_cache_tool/CacheTool.cc
Outdated
| #define VOL_HASH_ALLOC_SIZE (8 * 1024 * 1024) // one chance per this unit | ||
| #define VOL_HASH_EMPTY 0xFFFF | ||
| #define STORE_BLOCK_SHIFT 13 | ||
| #define STORE_BLOCK_SIZE 8192 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed, handled by Scalar.
cmd/traffic_cache_tool/CacheTool.cc
Outdated
|
|
||
| struct Stripe; | ||
|
|
||
| std::vector<Stripe *> globalVec_stripe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a member of Cache,not a global.
cmd/traffic_cache_tool/CacheTool.cc
Outdated
| struct Stripe; | ||
|
|
||
| std::vector<Stripe *> globalVec_stripe; | ||
| std::unordered_set<std::string> URLset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be a global, it should be embedded in the command function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
cmd/traffic_cache_tool/CacheTool.cc
Outdated
|
|
||
| std::vector<Stripe *> globalVec_stripe; | ||
| std::unordered_set<std::string> URLset; | ||
| unsigned short *stripes_hash_table; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a member, not a global.
|
|
||
| int64_t _buckets; ///< Number of buckets per segment. | ||
| int64_t _segments; ///< Number of segments. | ||
| const char *hashText = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a std::string.
|
|
||
| Stripe::Stripe(Span *span, Bytes start, CacheStoreBlocks len) : _span(span), _start(start), _len(len) | ||
| { | ||
| const char *diskPath = span->_path.path(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this used for, that _span->_path.path() can't be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No use of this. I think I was looking for a shorter variable name
cmd/traffic_cache_tool/CacheTool.cc
Outdated
| char *hash_text = static_cast<char *>(ats_malloc(hash_text_size)); | ||
| strncpy(hash_text, diskPath, hash_text_size); | ||
| snprintf(hash_text + hash_seed_size, (hash_text_size - hash_seed_size), " %" PRIu64 ":%" PRIu64 "", (uint64_t)_start, | ||
| (uint64_t)_len.count() * STORE_BLOCK_SIZE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be _len. If not, we'll need to update the version of Scalar here.
| const char *diskPath = span->_path.path(); | ||
| const size_t hash_seed_size = strlen(diskPath); | ||
| const size_t hash_text_size = hash_seed_size + 32; | ||
| char *hash_text = static_cast<char *>(ats_malloc(hash_text_size)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be std::string.
| Cache::allocStripe(Span *span, int vol_idx, CacheStripeBlocks len) | ||
| { | ||
| auto rv = span->allocStripe(vol_idx, len); | ||
| std::cout << span->_path << ":" << vol_idx << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just temporary debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
| return zret; | ||
| } | ||
|
|
||
| Errata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why load them in a table?
cmd/traffic_cache_tool/CacheTool.cc
Outdated
| uint64_t x = elt->hash_id.fold(); | ||
| // seed random number generator | ||
| rnd[i] = (unsigned int)x; | ||
| total += ((elt->_len.count() * STORE_BLOCK_SIZE) >> STORE_BLOCK_SHIFT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a no-op?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah.. I calculated this to verify with the ATS implementation
3106d53 to
e8846f8
Compare
a29c7dd to
962ee45
Compare
Add URL stripe assignment simulation.
YTSATS-2687: Align enums for hooks
Fix the `make check` failure with the following output by changing to .gold which meets our license exclusion regex rules: Printing headers for text files without a valid license header... ===================================================== == File: ./tests/gold_tests/records/gold/renamed_records.out ===================================================== ``` ┌■ 8 Renamed records: └┬──» #1 : proxy.config.output.logfile -> proxy.config.output.logfile.name ├──» #2 : proxy.config.exec_thread.autoconfig -> proxy.config.exec_thread.autoconfig.enabled ├──» #3 : proxy.config.hostdb -> proxy.config.hostdb.enabled ├──» #4 : proxy.config.tunnel.prewarm -> proxy.config.tunnel.prewarm.enabled ├──» #5 : proxy.config.ssl.origin_session_cache -> proxy.config.ssl.origin_session_cache.enabled ├──» #6 : proxy.config.ssl.session_cache -> proxy.config.ssl.session_cache.value ├──» #7 : proxy.config.ssl.TLSv1_3 -> proxy.config.ssl.TLSv1_3.enabled └──» #8 : proxy.config.ssl.client.TLSv1_3 -> proxy.config.ssl.client.TLSv1_3.enabled ```
Don't merge this please. I have a lot of debug prints here