Skip to content
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

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

Merged
merged 5 commits into from
Aug 16, 2023

Conversation

linh2931
Copy link
Member

@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
Member

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?

}

eosvmoc::code_cache_async cc;

// 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
Member

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
Member 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
Member 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
Member

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