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

Adding BufferConfig and interface for specifying CustomAllocator(s) #79

Merged
merged 7 commits into from
Sep 29, 2020

Conversation

adamhass
Copy link
Collaborator

@adamhass adamhass commented Sep 23, 2020

(Maybe some of this will be up for discussion tomorrow so I figured it's good to get the PR up before then)

The main change is that I've removed the Constant parameters from the buffers and made them all configurable parameters. They're configurable through the struct BufferConfig.

We want to keep Kompact decoupled from the NetworkDispatcher, such that users may specify a different NetworkDispatcher, which might use a different NetworkThread/executor.

There are three places Buffers are used: 1. NetworkThread for receiving messages, 2. NetworkDispatcher for lazy serialisation on the outbound path, and 3. Actors for eager serialisation on the outbound path.

There are three Configurations in play here

  1. KompactConfig containing a Hocon config into which custom configuration parameters can be inserted and which Actors have access to. (afaik actors don't have access to the KompactConfig struct at runtime, only the Hocon object, but this could be changed)

  2. NetworkConfig which is not part of KompactConfig and Actors do not have access to. (right now they will use the same config, can easily be changed)

  3. BufferConfig which may be injected into and parsed from the KompactConfigs Hocon, and is also a subset of the NetworkConfig.

In this PR the NetworkDispatchers lazy-serialisation buffers and the NetworkThreads buffers will pull the BufferConfig from the NetworkConfig, and if unspecified will use the default values.

Actors on the other hand may use the explicit init method:
self.ctx.borrow().init_buffers(Option<BufferConfig>, Option<Arc<CustomAllocator>>)
If they have not already used their buffers when that call is made they will initialize their buffers with the options.
If they try to use their buffers before calling that method, it will be called implicitly with (None, None).
-In that case the Actors BufferConfig will be pulled from the KompactConfigs Hocon config, if it fails to fetch the BufferConfig from the Hocon config, the default values will be used. If no CustomAllocator is specified they will use the Default allocator.

To launch a system where all buffers use a CustomAllocator and different BufferConfig's

First the NetworkDispatcher has the be created with a NetworkConfig:
NetworkConfig::with_custom_allocator(addr, buffer_config_n.clone(), custom_allocator.clone())

Then all Actors of the system must call the method: self.ctx.borrow().init_buffers(Some(buffer_config_n), Some(custom_allocator)) before they attempt to use tell_serialised(...).

The CustomAllocator stuff is WIP and is untested atm, but I believe it's fully possible to plug in a CustomAllocator at the appropriate places and it will work. I think we will need to discuss CustomAllocator stuff further before spending too much time on it right now is warranted. For example, what relation the CustomAllocator has to the BufferConfig?

The rest of the testing could do with improvements but I believe the basic functionality of BufferConfiguration and their priorities are covered right now.

@Bathtor
Copy link
Contributor

Bathtor commented Sep 23, 2020

The build failure on nightly seems to be caused by this: async-rs/async-std#883.

Can you change the travis config to add an earlier nightly and mark the latest one as allowed to fail (so we know once it's fixed and we can remove that code again)?

core/Cargo.toml Outdated Show resolved Hide resolved
core/src/net/network_thread.rs Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
core/src/dispatch/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Bathtor Bathtor left a comment

Choose a reason for hiding this comment

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

Also here:

    pub(crate) initial_pool_count: usize,
    /// Specifies the maximum amount of `BufferChunk`s individual `BufferPool`s will allocate
    pub(crate) max_pool_count: usize,
    /// Minimum number of bytes an `EncodeBuffer` must have available before serialisation into it
    pub(crate) encode_min_remaining: usize,

I think they would better be called initial_chunk_count, max_chunk_count and encode_buf_min_free_space

core/src/net/buffer.rs Outdated Show resolved Hide resolved
@Bathtor
Copy link
Contributor

Bathtor commented Sep 23, 2020

Ok the nightly issue is already fixed on master (rust-lang/rust#77089) so I guess it's not worth to continue fiddling with the travis file. Maybe just revert the changes and we wait until the new nightly comes out.

@Bathtor Bathtor added this to the 0.10.0 milestone Sep 24, 2020
@Bathtor
Copy link
Contributor

Bathtor commented Sep 24, 2020

Add a tutorial section for the new APIs in this PR. (As discussed during the roadmap meeting)

@Bathtor
Copy link
Contributor

Bathtor commented Sep 28, 2020

Meh, travis is running out of log space again. Gotta remove DEBUG output again and also maybe move INFO Got msg PingMsg { i: 0 } all of these to DEBUG level instead of INFO.

…atures tested in test-feature-matrix, changed the test_actors info! prints to debug!
@Bathtor Bathtor merged commit 248f5e1 into kompics:master Sep 29, 2020
@adamhass adamhass deleted the network_config_pr branch October 5, 2020 09:38
@adamhass adamhass mentioned this pull request Oct 5, 2020
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.

2 participants