Skip to content

Add countSerializeByteSize/serializeToPos/deserializeAndInsertFromPos interface to IColumn#9553

Merged
ti-chi-bot[bot] merged 52 commits intopingcap:masterfrom
gengliqi:de-serialize-byte
Nov 21, 2024
Merged

Add countSerializeByteSize/serializeToPos/deserializeAndInsertFromPos interface to IColumn#9553
ti-chi-bot[bot] merged 52 commits intopingcap:masterfrom
gengliqi:de-serialize-byte

Conversation

@gengliqi
Copy link
Contributor

@gengliqi gengliqi commented Oct 24, 2024

What problem does this PR solve?

Issue Number: ref #9060

Problem Summary:

What is changed and how it works?

Some code from #9191.

Add countSerializeByteSize/serializeToPos/deserializeAndInsertFromPos interface to IColumn

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

u
Signed-off-by: gengliqi <gengliqiii@gmail.com>
u
Signed-off-by: gengliqi <gengliqiii@gmail.com>
@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 24, 2024
Signed-off-by: gengliqi <gengliqiii@gmail.com>
Signed-off-by: gengliqi <gengliqiii@gmail.com>
Signed-off-by: gengliqi <gengliqiii@gmail.com>
Signed-off-by: gengliqi <gengliqiii@gmail.com>
@gengliqi gengliqi changed the title Add countSerializeByteSize/serializeToPos/deserializeAndInsertFromPos interface for IColumn Add countSerializeByteSize/serializeToPos/deserializeAndInsertFromPos interface to IColumn Oct 31, 2024
{
return i == 0 ? getOffsets()[0] : (getOffsets()[i] - getOffsets()[i - 1]);
}
size_t ALWAYS_INLINE sizeAt(ssize_t i) const { return (getOffsets()[i] - getOffsets()[i - 1]); }
Copy link
Contributor

@JaySon-Huang JaySon-Huang Nov 1, 2024

Choose a reason for hiding this comment

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

Does we ensure getOffsets()[-1] == 0 is well-defined?

Copy link
Contributor Author

@gengliqi gengliqi Nov 1, 2024

Choose a reason for hiding this comment

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

Signed-off-by: gengliqi <gengliqiii@gmail.com>
@gengliqi
Copy link
Contributor Author

gengliqi commented Nov 7, 2024

/retest

Signed-off-by: gengliqi <gengliqiii@gmail.com>
Signed-off-by: gengliqi <gengliqiii@gmail.com>
Signed-off-by: gengliqi <gengliqiii@gmail.com>
Signed-off-by: gengliqi <gengliqiii@gmail.com>
Signed-off-by: gengliqi <gengliqiii@gmail.com>
Signed-off-by: gengliqi <gengliqiii@gmail.com>
Signed-off-by: gengliqi <gengliqiii@gmail.com>
u
Signed-off-by: gengliqi <gengliqiii@gmail.com>
t
Signed-off-by: gengliqi <gengliqiii@gmail.com>
u
Signed-off-by: gengliqi <gengliqiii@gmail.com>
Signed-off-by: gengliqi <gengliqiii@gmail.com>
tmp_val.resize(limb_count, limb_count);
inline_memcpy(tmp_val.limbs(), val.limbs(), limb_count * sizeof(boost::multiprecision::limb_type));
if (val.sign() != tmp_val.sign())
tmp_val.negate();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use the builtin assign function like tmp_val.assign(val)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done.

data.resize(prev_size + (size - i) / avx2_width * avx2_width, FULL_VECTOR_SIZE_AVX2);
for (; i + avx2_width <= size; i += avx2_width)
{
/// Loop unrolling
Copy link
Contributor

Choose a reason for hiding this comment

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

You mean this is a todo item or the compiler will do the loop unrolling?

Copy link
Contributor Author

@gengliqi gengliqi Nov 20, 2024

Choose a reason for hiding this comment

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

The compiler will probably do the loop unrolling (if av2_width is not too large) because avx2_width is a constexpr and there is no dependency between each loop.

static constexpr size_t pad_right = integerRoundUp(pad_right_, ELEMENT_SIZE);
/// pad_left is also rounded up to 16 bytes to maintain alignment of allocated memory.
static constexpr size_t pad_left = integerRoundUp(integerRoundUp(pad_left_, ELEMENT_SIZE), 16);
/// pad_left is also rounded up to 64 bytes to maintain alignment of allocated memory.
Copy link
Contributor

Choose a reason for hiding this comment

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

why need to change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If not, the data will not be aligned to 64 bytes when calling resize(n, 64).

Signed-off-by: gengliqi <gengliqiii@gmail.com>
Copy link
Contributor

@windtalker windtalker left a comment

Choose a reason for hiding this comment

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

lgtm

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 20, 2024
@gengliqi gengliqi requested a review from guo-shaoge November 20, 2024 10:03
@gengliqi
Copy link
Contributor Author

/hold

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2024
@gengliqi
Copy link
Contributor Author

/unhold

@ti-chi-bot ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2024
size_t sz = size();
for (size_t i = 0; i < sz; ++i)
RUNTIME_CHECK_MSG(
sizeAt(i) > UINT32_MAX,
Copy link
Contributor

@guo-shaoge guo-shaoge Nov 21, 2024

Choose a reason for hiding this comment

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

Looks like you want to find the first string whose length is greater than UINT32_MAX?
But if sizeAt(i) > UINT32_MAX is true, for loop will iter to next value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed.

Signed-off-by: gengliqi <gengliqiii@gmail.com>
@ti-chi-bot ti-chi-bot bot added the lgtm label Nov 21, 2024
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guo-shaoge, windtalker

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [guo-shaoge,windtalker]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 21, 2024
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 21, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-11-20 08:36:58.752071848 +0000 UTC m=+20806.371726363: ☑️ agreed by windtalker.
  • 2024-11-21 08:10:22.289998015 +0000 UTC m=+105609.909652532: ☑️ agreed by guo-shaoge.

@gengliqi
Copy link
Contributor Author

/hold

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2024
Signed-off-by: gengliqi <gengliqiii@gmail.com>
@gengliqi
Copy link
Contributor Author

/unhold

@ti-chi-bot ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2024
@ti-chi-bot ti-chi-bot bot merged commit fab294a into pingcap:master Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants