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

Oak shared config #196

Merged
merged 9 commits into from
Jan 5, 2022
Merged

Conversation

liran-funaro
Copy link
Contributor

@liran-funaro liran-funaro commented Dec 28, 2021

fixes #194

  • Oak shared config
  • Allow multiple Oak instantiations with one builder
  • Fix more memory leaks in tests
  • Fix bad assert pattern in tests

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Copy link
Contributor

@sanastas sanastas left a comment

Choose a reason for hiding this comment

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

The change looks good to me, some questions/comments inside

// to compare serilized and object keys
protected final OakSharedConfig<K, V> config;

// to compare serialized and object keys
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't comparator part of the config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each instance holds its own references to the "config items" it needs.
I implemented it that way to avoid another pointer dereference each time we access these elements.
In addition, the config object is stored here to be passed to the child Chunk instance.

// in split/compact process, represents parent of split (can be null!)
private final AtomicReference<BasicChunk<K, V>> creator;
// chunk can be in the following states: normal, frozen or infant(has a creator)
private final AtomicReference<State> state;
private final AtomicReference<Rebalancer<K, V>> rebalancer;
private final AtomicInteger pendingOps;
private final int maxItems;
protected final int maxItems;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to use getMaxItems()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems more natural to me to access internal final fields directly. I don't see why we need a function call here.
For the same reason, we access externalSize and statistics directly.

core/src/main/java/com/yahoo/oak/EntryArray.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/EntryArray.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/HashChunk.java Outdated Show resolved Hide resolved

private final OakSerializer<K> keySerializer;
private final OakSerializer<V> valueSerializer;
protected final MemoryManager keysMemoryManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why to keep the config and all those fields? In order to avoid de-referencing? I don't believe we will see any performance differences from accessing config.keysMemoryManager... We may try the benchmarks....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comparators are accessed frequently. Are you sure?
I think we can have a different PR after this one where we test this hypothesis.

core/src/main/java/com/yahoo/oak/OakMap.java Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/OakMap.java Show resolved Hide resolved
@liran-funaro liran-funaro merged commit 9de4de0 into yahoo:master Jan 5, 2022
@liran-funaro liran-funaro deleted the oak-shared-config branch January 5, 2022 10:57
@fujian-zfj
Copy link

Excuse me. I have some questions about the oak shared config.
I used to think that all oakMaps created by one OakMapBuilder could share the same BlockMempryAllocator, but the current design doesn't. I think that current design brings a probelm. When I use the same OakMapBuilder to create thousands of OakMaps in a multi-threaded environment, the BlockMemoryAllocator will be instantiated multiple times. At the same time, the problem is that thousands of 128M Block will be created in multi-threaded environment, and soon the memory is exhausted.
image

@fujian-zfj
Copy link

fujian-zfj commented Jan 20, 2022

Could all oakMap created by one OakMapBuilder share the same BlockMempryAllocator?
Because in real production, it is quit frequent to create thousands of OakMaps in a multi-threaded environment.

@sanastas
Copy link
Contributor

Hi @fujian-zfj

Thanks for your valuable suggestion. Indeed, we currently have one NativeMemoryAllocator per OakHash or OakMap instance. All memory allocators are using off-heap memory blocks from the same pool. It is possible to make the Oak instances to share same NativeMemoryAllocator (which is multi-threaded), but code needs to be upfated during closing, so closing one Oak instance doesn't close allocator for all others. Also too many Oak instances per memory allocator can cause threads contention and decrease the performance.

Anyway, in your case, a possible solution can be to decrease the block size. Use builder.setPreferredBlockSize() before any Oak instance is initiated.

Alternatively, if you would like to contribute some code to Oak, we can guide you to make the change yourself. You can get direct and clear instructions what needs to be done, code review and etc. This is also a possibility :)

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.

Can I use one OakMapBuilder to create more than one OakMap?
3 participants