Skip to content
This repository has been archived by the owner on Feb 3, 2023. It is now read-only.

Fixing pickledb bugs #1202

Merged
merged 29 commits into from
Apr 3, 2019
Merged

Fixing pickledb bugs #1202

merged 29 commits into from
Apr 3, 2019

Conversation

StaticallyTypedAnxiety
Copy link
Contributor

Please check one of the following, relating to the CHANGELOG, which should be updated if relevant

  • my changes to the code affect some exposed aspect of the developer experience, and I have added an item to relevant 'Added, Fixed, Changed, Removed, Deprecated, or Security' heading under the 'Unreleased' heading of the CHANGELOG, with the format - summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)
  • my changes to the code do not affect any exposed aspect of the developer experience

@StaticallyTypedAnxiety StaticallyTypedAnxiety changed the title Commited some changes Set pickledb in nodejs conductor Apr 2, 2019
@StaticallyTypedAnxiety
Copy link
Contributor Author

StaticallyTypedAnxiety commented Apr 3, 2019

Dont merge until the profiler is run(can still review) just need to confirm that the space issue has been fixed.

Edit : This has been confirmed, investigating why pickledb is 3 times larger than file.

Findings : File is larger because a further encoding is done on the content which is an encoded type in itself. This sounds like something that should be talked about architecturally

@@ -83,11 +85,13 @@ fn make_config(instance_data: Vec<InstanceData>, logger: LoggerConfiguration) ->

let agent_id = agent_config.id.clone();
let dna_id = dna_config.id.clone();
let temp = tempdir().expect("test was supposed to create temp dir");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was looking up tempdir's docs because I was wondering if that is a unique directory since we don't want multiple test instances share the same CAS. It says that the tempdir gets removed when TempDir is dropped. This happens a few lines below, right? The conductor will recreate the the path I guess but then nothing will delete it, so it actually is not temporary. This might cause mysterious problems later on.. Not sure if we actually want to merge pickle as default for nodejs conductor into develop.. We might want to if it really is faster than MemoryStorage..

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 agree, I think since memory is the fastest and this is for the use in the nodejs we should still go back to memory. I will revert this back asap

@StaticallyTypedAnxiety
Copy link
Contributor Author

Benchmarks have been run and flame graph is up on realtimeboard

test bench_file_eav_many_to_one ... bench: 183,641,550 ns/iter (+/- 131,852,367)
test bench_file_eav_one_to_many ... bench: 201,104,300 ns/iter (+/- 119,277,360)
test bench_memory_eav_many_to_one ... bench: 791,201 ns/iter (+/- 77,859)
test bench_memory_eav_one_to_many ... bench: 827,959 ns/iter (+/- 95,228)
test bench_pickle_eav_many_to_one ... bench: 14,167,780 ns/iter (+/- 14,597,584)
test bench_pickle_eav_one_to_many ... bench: 13,985,950 ns/iter (+/- 10,538,156)

Copy link
Collaborator

@lucksus lucksus left a comment

Choose a reason for hiding this comment

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

Cool! Hope this gets merged asap! :)
Maybe we should change the title of this PR though since it's now only fixing pickledb without setting it in the nodejs conductor.

@StaticallyTypedAnxiety StaticallyTypedAnxiety changed the title Set pickledb in nodejs conductor Fixing pickledb bugs Apr 3, 2019
@StaticallyTypedAnxiety StaticallyTypedAnxiety merged commit 40521bb into develop Apr 3, 2019
@zippy zippy deleted the pickle-db-bugs branch July 5, 2019 13:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants