-
Notifications
You must be signed in to change notification settings - Fork 138
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
fix: generate AppKey only once #469
Conversation
953c072
to
b023c81
Compare
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.
Hi @dorjesinpo, I have a general comment regarding AppInfos
. This structure is built with a default allocator in many places. It was also like this before this PR. Do you think it's worth to pass allocators to it?
My main concern is that we have many allocations with a default allocator and basically don't know where exactly it happened. In this sample, 2/3 of all allocations are done with a default allocator:
:::::::::: :::::::::: ALLOCATORS >> Last snapshot was 59.100 s ago.
| Bytes Allocated| -delta-| Max Bytes Allocated| Allocations| -delta- | Deallocations| -delta-
-----------------------+----------------+--------+--------------------+------------+-----------+--------------+-----------
allocators | 629,997,584| 52,112| 638,185,456| 276,423,010| 29,789,132| 276,163,685| 29,789,129
*direct* | 1,584| | 1,584| 10| | 1|
Application | 629,514,640| 52,176| 637,636,240| 91,877,218| 9,828,407| 91,618,220| 9,828,403
*direct* | 9,376| | 10,816| 34| | 7|
Dispatcher | 268,846,352| 448| 280,176,720| 55,490,162| 6,015,987| 55,474,332| 6,015,983
BufferFactory | 181,752,480| | 181,752,480| 1,365| | 0|
ClusterCatalog | 117,951,088| | 117,951,088| 509| | 215|
*direct* | 2,544| | 2,544| 15| | 2|
cl3 | 117,948,544| | 117,948,544| 494| | 213|
BlobSpPool | 45,438,048| 51,728| 45,438,336| 382,805| 447| 148,709| 447
PushElementsPool | 13,182,960| | 13,182,960| 5,090| | 0|
StatController | 1,519,920| | 1,525,152| 1,976| | 201|
*direct* | 413,328| | 415,296| 605| | 58|
DomainQueuesStats| 610,816| | 611,888| 641| | 15|
ClientsStats | 157,968| | 160,160| 206| | 19|
ClusterNodesStats| 134,576| | 134,576| 246| | 84|
ClustersStats | 131,168| | 131,168| 173| | 22|
DomainsStats | 51,200| | 51,200| 73| | 3|
BrokerStats | 20,864| | 20,864| 32| | 0|
TransportManager | 707,520| | 815,520| 35,995,194| 3,811,973| 35,994,713| 3,811,973
*direct* | 706,048| | 814,048| 35,995,164| 3,811,973| 35,994,704| 3,811,973
Channel-node2 | 640| | 752| 16| | 9|
Channel-node4 | 416| | 416| 7| | 0|
Channel-node0 | 416| | 416| 7| | 0|
SessionNegotiator | 105,120| | 154,288| 73| | 43|
DomainManager | 1,776| | 1,776| 10| | 0|
Task | 411,888| | 412,944| 250| | 120|
Default Allocator | 48,704| -64| 115,376| 184,545,445| 19,960,725| 184,545,303| 19,960,726
EventScheduler | 11,936| | 12,080| 20| | 4|
Global Allocator | 8,832| | 16,128| 67| | 37|
b023c81
to
4f8c3fc
Compare
Indeed. We allocate in static |
@dorjesinpo if passing an allocator requires refactor which might make FSM work complicated, let's not do it now then |
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.
Build 320 of commit 4f8c3fc has completed with FAILURE
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.
I still see several places where we are passing in empty appIds in legacy mode. Please address them.
74094ce
to
22af805
Compare
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.
Code looks good. Did you test the changes? Do you need to wait for e_STRONG
consistency config PR for the ledger?
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
d535271
to
8daad88
Compare
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
8daad88
to
5535927
Compare
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.
Build 332 of commit 5535927 has completed with FAILURE
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.
lgtm
Upon queue registration in the storage, use keys in the CSL instead of generating new ones to keep CSL and non-CSL consistent.
Secondary, switching to
unordered_set
fromvector
to optimize calculating Apps diff