Use minimal TLS implementation for thread-local mapping objects

[senderista]

In order to avoid an expensive worst-case TLS implementation of the thread-local variables holding shared-memory mappings, we needed to 1) make all constructors and destructors of the mapping objects trivial, and 2) use the (nonstandard, but supported by clang and gcc) __thread attribute (rather than the C++11 thread_local keyword) on these thread-local variables to statically guarantee that the minimal TLS implementation was used.

I verified that the finalizer methods on these mapping objects (invoked by data_mapping_t::close()) were reliably called in all cases where mappings might have been leaked, in both the client and server.

Performance improvements are dramatic (10^8 records, asserts disabled):

test_insert_perf_basic.simple_table_insert: 0.34us -> 0.31us
test_update_perf_basic.simple_table_update: 0.88us -> 0.59us
test_update_perf_basic.simple_table_update_dynamic: 0.89us -> 0.62us
test_read_perf_basic.table_scan: 0.17us -> 0.09us
test_read_perf_basic.table_scan_data_access: 0.24us -> 0.12us
test_read_perf_rel.single_join: 0.18us -> 0.08us
test_read_perf_rel.nested_joins (10^6 records): 0.19us -> 0.09us

[daxhaw]

Cool!

[simone-gaia]

LGTM, great stuff!

[LaurentiuCristofor]

Performance improvements are dramatic (10^8 records, asserts disabled):

What does this mean? Were all asserts disabled or just specific ones (the ones recently converted to debug)? Also, what was the gain without disabling any new asserts?

If there is a great gain from disabling asserts, which asserts accounted for it (i,e, which ones would be good candidates for being changed into debug asserts)?

[senderista]

I disabled all asserts to help determine the magnitude of performance improvements (because assertion overhead will tend to dilute perf gains). Of course asserts were disabled in the baseline (master) as well, if you were wondering. I haven't measured with asserts enabled.

[LaurentiuCristofor]

I think that close_internal was only called in two places, so if this goes away, can we merge the internal function into the other caller?

[LaurentiuCristofor]

Oh, and I don't remember - how do we call this now - where is this cleanup being done after this change? Is it through the data_mapping functions?

We should also add an explicit comment to this class that the cleanup is manual and no longer automatic.

[senderista]

Yes, all cleanup is being done through data_mapping_t::close(); I can add a comment to that effect. These classes could use some refactoring as you mentioned before, but I considered that out of scope for this PR; feel free to change as you see fit. (BTW, the only virtual calls I found were through the data_mapping_t wrappers, and they're not in hot paths, so I left them alone.)

[senderista]

In violation of my stated goal of minimal changes to the data mapping classes, I went ahead and removed the close_internal() method :) Given the now-empty destructor, I thought it was superfluous to the point of being confusing.

[LaurentiuCristofor]

These classes could use some refactoring as you mentioned before, but I considered that out of scope for this PR; feel free to change as you see fit. (BTW, the only virtual calls I found were through the data_mapping_t wrappers, and they're not in hot paths, so I left them alone.)

I'm not sure if there is much refactoring that can be done here. That's what I noticed also, when I took a closer look - that the hierarchy was needed for the data_mapping_t handling.

In violation of my stated goal of minimal changes to the data mapping classes, I went ahead and removed the close_internal() method :) Given the now-empty destructor, I thought it was superfluous to the point of being confusing.

Right. Thanks! It's a fairly small change and I think it helps to remove these "internal" helpers when they're no longer needed.

[LaurentiuCristofor]

I think this is fine, but I would add a comment to explain the loss of automatic cleanup in these classes. I'd put that at the top of the mapped_data.hpp file. It might also be worth mentioning that all instances are typically handled through the data_mapping_t class - I don't think we have any individual instances elsewhere, anymore (only in tests, perhaps).