Skip to content

Set specific sliced pages for read-only threads in EOS VM OC Tierup #1498

Merged
linh2931 merged 5 commits intomainfrom
oc_tier_ro_thread_mem_fonfig
Aug 16, 2023
Merged

Set specific sliced pages for read-only threads in EOS VM OC Tierup #1498
linh2931 merged 5 commits intomainfrom
oc_tier_ro_thread_mem_fonfig

Conversation

@linh2931
Copy link
Contributor

@linh2931 linh2931 commented Aug 15, 2023

#1488 missed setting specific sliced pages for read-only threads in OC tierup.

This PR

  • sets sliced pages for main and read-only threads correctly in OC tierup
  • makes sure sliced pages are not set up in OC tierup and OC runtime at the same time by using unique_ptr.

Before the PR, nodeos running the integration test read-only-trx-parallel-eos-vm-oc-test (with 6 read-only threads) uses 29 TB virtual memory; after the PR, it uses 4.7 TB virtual memory as expected.

@heifner
Copy link
Contributor

heifner commented Aug 15, 2023

Before the PR, nodeos running the integration test read-only-trx-parallel-eos-vm-oc-test (with 6 read-only threads) uses 29 TB virtual memory; after the PR, it uses 4.7 TB virtual memory as expected.

Can you add a verification to the test?

// Each thread requires its own exec and mem.
thread_local static std::unique_ptr<eosvmoc::executor> exec;
thread_local static eosvmoc::memory mem;
thread_local static std::unique_ptr<eosvmoc::memory> mem;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(obviously not in this PR) but I'd really like for us to noodle on how to move away from these thread_locals, they have dangerous lifetime semantics imo. In this particular example, when eosvmoc_tier is destroyed, cc is destroyed, but the exec created with a reference to that cc lives on. So if executor's dtor (which will run well after eosvmoc_tier's dtor) touches the code_cache_async that was passed to it via reference: bad news! It's a real trap -- passing a reference to some object to the ctor of an object implies that reference will be valid for the lifetime of the object.. but it's not true here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will be looking into moving away from those thread_locals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #1503 to track this. Will work on it soon.

… as the number of read-only threads increases

def verifyOcVirtualMemory():
try:
with open(f"/proc/{apiNode.pid}/statm") as f:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize it doesn't matter now since OC is linux only, but it might be nice to skip this test on non-Linux if that's easy to do.

@linh2931 linh2931 merged commit 949c7c4 into main Aug 16, 2023
@linh2931 linh2931 deleted the oc_tier_ro_thread_mem_fonfig branch August 16, 2023 18:00
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