[
https://issues.apache.org/jira/browse/GEODE-8756?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17250453#comment-17250453
]
ASF GitHub Bot commented on GEODE-8756:
---------------------------------------
pivotal-jbarrett commented on a change in pull request #703:
URL: https://github.com/apache/geode-native/pull/703#discussion_r544465839
##########
File path: cppcache/src/CacheableString.cpp
##########
@@ -119,8 +119,18 @@ bool CacheableString::isAscii(const std::string& str) {
}
size_t CacheableString::objectSize() const {
- auto size = sizeof(CacheableString) +
- sizeof(std::string::value_type) * m_str.capacity();
+ auto size = sizeof(CacheableString);
+
+ // This is calculated in order not to count more bytes than necessary
+ // whenever SSO applies.
+ auto delta = reinterpret_cast<const uint8_t*>(m_str.data()) -
+ reinterpret_cast<const uint8_t*>(&m_str);
+ if (delta >= static_cast<decltype(delta)>(sizeof(decltype(m_str))) ||
+ delta < 0) {
+ // Add an extra character for the null-terminator
+ size += sizeof(decltype(m_str)::value_type) * (m_str.capacity() + 1UL);
Review comment:
Very subtle find on the null terminator!
> Memory locations obtained from the allocator but not available for storing
any element are not counted in the allocated storage. Note that the null
terminator is not an element of the basic_string.
##########
File path: cppcache/test/CacheableStringTests.cpp
##########
@@ -224,4 +224,28 @@ TEST_F(CacheableStringTests, TestFromDataNonAsciiHuge) {
EXPECT_EQ(utf8, str->value());
}
+class CacheableStringSizeTests : public ::testing::TestWithParam<std::size_t> {
+};
+
+INSTANTIATE_TEST_SUITE_P(CacheableStringTests, CacheableStringSizeTests,
+ ::testing::Values(0UL, 1UL, 3UL, 7UL, 15UL, 23UL,
31UL,
+ 47UL, 63UL, 1025UL, 4097UL));
+
+TEST_P(CacheableStringSizeTests, TestObjectSize) {
+ auto size = GetParam();
+ std::string str(size, '_');
+ auto cacheable = CacheableString::create(str);
+ EXPECT_TRUE(cacheable);
+
+ auto expected_size = sizeof(CacheableString);
Review comment:
If we are literally copying the internal logic of the method is it
really a valid test? Is there another way test the response?
Could we provide a customized allocator that we use for checking
allocations? 🤷
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> CacheableString objectSize is not correct
> -----------------------------------------
>
> Key: GEODE-8756
> URL: https://issues.apache.org/jira/browse/GEODE-8756
> Project: Geode
> Issue Type: Sub-task
> Components: native client
> Affects Versions: 1.11.0, 1.12.0, 1.13.0, 1.13.1
> Reporter: Mario Salazar de Torres
> Assignee: Mario Salazar de Torres
> Priority: Major
> Labels: pull-request-available
>
> CacheableString objectSize function is returning an incorrect value.
> This class is based upon STL's string implementation, and most of the
> compilers implementations apply what's called SSO.
> What SSO basically does is if the string occupies less than a certain amount,
> no extra memory would be allocated in the heap, and the character-sequence
> would be stored in the object itself. This is typically achieved by using
> union semantics.
> Right now if SSO applies, objectSize calculates the size of std::string as
> sizeof(std::string) + m_str.capacity(), which is more than it actually
> occupies.
> On the other hand starting C++11 STL's strings needs to allocate an extra
> character
> to keep the null-terminator in the same buffer as the actual string. This is
> specified in section § 21.4.7.1 within the C++11 standard.
> Because of this objectSize should take the null-terminator into account,
> which was not the case.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
