Eliminate memory allocation from critical path:

[nbougalis]

When writing objects to the NodeStore, we need to convert them from the in-memory format to the binary format used by the node store.

The conversion is handled by the EncodedBlob class, which is only instantianted on the stack. Coupled with the fact that most objects are under 1024 bytes in size, this presents an opportunity to elide a memory allocation in a critical path.

This commit also makes the interface of EncodedBlob simpler while also eliminating a subtle corner case that could result in dangling pointers.

High Level Overview of Change

Context of Change

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (non-breaking change that only restructures code)
• Tests (You added tests for code that already exists, or your new feature included in this PR)
• Documentation Updates
• Release

[intelliot]

(note: would like this to be tested on some full-history servers)

[intelliot]

(run on 2 full history servers in production; see if there is reduced memory usage)

[mtrippled]

(run on 2 full history servers in production; see if there is reduced memory usage)

@manojsdoshi (I don't know Xun's git handle). Elliot is asking for this branch to be run on 2 full history servers and to compare memory usage before and after, please.

[nbougalis]

(run on 2 full history servers in production; see if there is reduced memory usage)

I don't expect we'll see significant reduction in memory usage for this PR. It simply eschews the use of and std::shared_ptr when it's unnecessary and tries to avoid using the heap whenever possible, opting to use stack-based memory allocation instead.

While I'm confident that this is a net gain both in terms of memory usage (lower fragmentation) and perfromance (less work to do at runtime), the reality is that those gains are probably not directly measurable.

Not saying you shouldn't run this to test; definitely do that. But I don't think the merging criterion should be whether there's observable reduction in memory usage. Tagging @mtrippled, @HowardHinnant for their thoughts.

I do have an idea for something that could significantly reduce memory usage (to the tune of ~600MB) and (theoretically) improve performance. It's the same principle as the #4218, but for NodeObject instead: reduce the size of NodeObject by allocating the dynamic buffer immediately after the object itself, switch from std::shared_ptr to boost::intrusive_ptr and leverage SlabAllocator to grab memory more efficiently. If someone wants to tackle that, please feel free. The biggest challenge will be the TaggedCache we use to cache NodeObject instances; that code relies on std::shared_ptr and std::weak_ptr so some rethinking would be necessary.

[mtrippled]

👍 LGTM

[intelliot]

@nbougalis if you feel this is ready to merge, please put the Passed label on it.