feat(Rust): support context_pool to reduce context allocation && support se/de in multi-thread #2737
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Btw, |
Contributor
Author
|
When in reset, will call functions named |
Contributor
Author
|
maybe I can set Fory::metastring_resolver to Arc<rwlock> |
Collaborator
Split into read and write, Then hold it on Read Write context |
Collaborator
use reset instead, it used in java/python |
Collaborator
|
@urlyy Nice work! Could you run fory-benchmarks and compare the performance with previous version? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
chaokunyang
reviewed
Oct 10, 2025
rust/fory-core/src/buffer.rs
Outdated
| impl Reader { | ||
| pub fn new(bf: &[u8]) -> Reader { | ||
| Reader { | ||
| bf: bf.to_vec(), |
Collaborator
There was a problem hiding this comment.
will this introduce a copy?
Contributor
Author
There was a problem hiding this comment.
yes, but only when enter a deserialize will do such a clone.
Contributor
Author
|
@chaokunyang Handling these |
Collaborator
@urlyy You can create a new struct to manage read state: pub struct ReadState {
pub meta_resolver: MetaReaderResolver,
meta_string_resolver: MetaStringReaderResolver,
pub ref_reader: RefReader,
}And pooling this class: struct Fory {
...,
read_context_pool: Pool<ReadState>,
}For deserialize, just create ReadContext everytime from ReadState, and use lifetime for |
Contributor
Author
|
Here is the output after replace type of |
Contributor
Author
|
@chaokunyang cargo-criterion can export output in |
Contributor
Author
|
btw, https://github.com/apache/fory/actions/runs/18417030950/job/52483058475?pr=2737, this CI takes too much time, 17 minutes! I can't bare this. |
Contributor
Author
|
Using raw pointers is unsafe, but as long as we don’t continue holding references after deserialization and only use the reference within a single thread, it won’t cause any issues. |
chaokunyang
approved these changes
Oct 11, 2025
2 tasks
chaokunyang
added a commit
that referenced
this pull request
Oct 15, 2025
## Why? Simplify the serialization interface for better ergonomics API ## What does this PR do? - Remove `Fory` from `Serializer` trait - Make `WriteContext` as only enviroment for serialization - Make `ReadContext` as only enviroment for deserialization - Use `OnceLock` for thread-safe, lazily initialization for `Pool` ## Related issues #2737 ## Does this PR introduce any user-facing change? <!-- If any user-facing interface changes, please [open an issue](https://github.com/apache/fory/issues/new/choose) describing the need to do so and update the document if necessary. Delete section if not applicable. --> - [ ] Does this PR introduce any public API change? - [ ] Does this PR introduce any binary protocol compatibility change? ## Benchmark <!-- When the PR has an impact on performance (if you don't know whether the PR will have an impact on performance, you can submit the PR first, and if it will have impact on performance, the code reviewer will explain it), be sure to attach a benchmark data here. Delete section if not applicable. -->
rajuyadav03
pushed a commit
to rajuyadav03/fory
that referenced
this pull request
Oct 20, 2025
…ort se/de in multi-thread (apache#2737) ## What does this PR do? 1. Remove the lifetime annotations from `WriteContext` and `ReadContext` because managing their lifetimes was too troublesome. The **trade-off** is that, previously, `MetaWriterResolver` held references to the corresponding `TypeDef` obtained from `TypeResolver`, and both contexts also fetched references to `Harness` objects managed by `TypeResolver`. Now, all these references have been replaced with `clone()` to simplify lifetime management. This may introduce a non-negligible performance overhead, but it was the only practical solution I could implement.🥲 2. Remove `&fory` from the member variables of `Context`. Instead, `&fory` is now passed in as a function parameter rather than being retrieved via `context.get_fory()`. This change modified many API signatures. 3. Add two pools to `Fory`’s member variables to allow reuse of both types of contexts. Tests confirmed that the same context addresses were reused multiple times. However, since automatic recycling would require `Arc<Fory>`, only manual recycling has been implemented so far — this operation is handled internally(within `serialize()/deserialize()`), and users don’t need to recycle contexts manually. Also, for the `de/serialize_with_context(context)` functions, if users call them directly, the user's manually managed contexts will **not** be returned to the pool. 4. Add `reset()` methods to be executed on `Context objects` before recycling. 5. Modified `TypeResolver::sorted_field_names_map` from `RefCell<...>` to `RwLock<...>`, and changed `MetaReaderResolver::reading_type_defs` from `Vec<Rc<TypeMeta>>` to `Vec<Arc<TypeMeta>>`. Split `Fory::MetaStringResolver` to `ReadContext::MetaStringResolver` and `WriteContext::MetaStringResolver`. In addition, `RefReader` was unsafely marked as `Send` and `Sync`. These changes allow `Fory` to support serialization across multiple threads. A concrete example can be found in `rust/tests/tests/test_multi_thread.rs`. However, I’m not sure whether using `Lock` and `Arc` will impact single-thread performance. But it may be troublesome to write another `ThreadSafeFory`. 6. The reason why I `unsafe impl Send/Sync for RefReader {}`, is that , `RefReader` contains `Box<dyn Any>`, which by default is not `Send`/`Sync`. But In our usage, each ref_reader is only accessed within a context, by `one` thread at a time, so it is safe to implement `Send`/`Sync` manually using `unsafe`. ## Related issues - close apache#2717 ## Does this PR introduce any user-facing change? yes, like implementing in EXT, need to pass a extra parameter `fory`.
rajuyadav03
pushed a commit
to rajuyadav03/fory
that referenced
this pull request
Oct 20, 2025
## Why? Simplify the serialization interface for better ergonomics API ## What does this PR do? - Remove `Fory` from `Serializer` trait - Make `WriteContext` as only enviroment for serialization - Make `ReadContext` as only enviroment for deserialization - Use `OnceLock` for thread-safe, lazily initialization for `Pool` ## Related issues apache#2737 ## Does this PR introduce any user-facing change? <!-- If any user-facing interface changes, please [open an issue](https://github.com/apache/fory/issues/new/choose) describing the need to do so and update the document if necessary. Delete section if not applicable. --> - [ ] Does this PR introduce any public API change? - [ ] Does this PR introduce any binary protocol compatibility change? ## Benchmark <!-- When the PR has an impact on performance (if you don't know whether the PR will have an impact on performance, you can submit the PR first, and if it will have impact on performance, the code reviewer will explain it), be sure to attach a benchmark data here. Delete section if not applicable. -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Remove the lifetime annotations from
WriteContextandReadContextbecause managing their lifetimes was too troublesome. The trade-off is that, previously,MetaWriterResolverheld references to the correspondingTypeDefobtained fromTypeResolver, and both contexts also fetched references toHarnessobjects managed byTypeResolver. Now, all these references have been replaced withclone()to simplify lifetime management. This may introduce a non-negligible performance overhead, but it was the only practical solution I could implement.🥲Remove
&foryfrom the member variables ofContext. Instead,&foryis now passed in as a function parameter rather than being retrieved viacontext.get_fory(). This change modified many API signatures.Add two pools to
Fory’s member variables to allow reuse of both types of contexts. Tests confirmed that the same context addresses were reused multiple times. However, since automatic recycling would requireArc<Fory>, only manual recycling has been implemented so far — this operation is handled internally(withinserialize()/deserialize()), and users don’t need to recycle contexts manually. Also, for thede/serialize_with_context(context)functions, if users call them directly, the user's manually managed contexts will not be returned to the pool.Add
reset()methods to be executed onContext objectsbefore recycling.Modified
TypeResolver::sorted_field_names_mapfromRefCell<...>toRwLock<...>, and changedMetaReaderResolver::reading_type_defsfromVec<Rc<TypeMeta>>toVec<Arc<TypeMeta>>. SplitFory::MetaStringResolvertoReadContext::MetaStringResolverandWriteContext::MetaStringResolver. In addition,RefReaderwas unsafely marked asSendandSync. These changes allowForyto support serialization across multiple threads. A concrete example can be found inrust/tests/tests/test_multi_thread.rs. However, I’m not sure whether usingLockandArcwill impact single-thread performance. But it may be troublesome to write anotherThreadSafeFory.The reason why I
unsafe impl Send/Sync for RefReader {}, is that ,RefReadercontainsBox<dyn Any>, which by default is notSend/Sync. But In our usage, each ref_reader is only accessed within a context, byonethread at a time, so it is safe to implementSend/Syncmanually usingunsafe.Related issues
Does this PR introduce any user-facing change?
yes, like implementing in EXT, need to pass a extra parameter
fory.