Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve the preformance for hgraph bulid & add #216

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Conversation

LHT129
Copy link
Collaborator

@LHT129 LHT129 commented Dec 13, 2024

  • avx512 simd optimization
  • memory_block_io optimization

@LHT129 LHT129 added the kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) label Dec 13, 2024
@LHT129 LHT129 requested a review from inabao December 13, 2024 03:34
@LHT129 LHT129 self-assigned this Dec 13, 2024
@@ -37,10 +37,17 @@ class MemoryBlockIO : public BasicIO<MemoryBlockIO> {
public:
explicit MemoryBlockIO(Allocator* allocator, uint64_t block_size = DEFAULT_BLOCK_SIZE)
: block_size_(block_size), allocator_(allocator), blocks_(0, allocator) {
block_bit_ = 0;
while (block_size_ > 0) {
block_size_ >>= 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

will block_size_ be set to 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

never

return blocks_[block_no] + block_off;
}

[[nodiscard]] inline bool
check_in_one_block(uint64_t off1, uint64_t off2) const {
return (off1 / block_size_) == (off2 / block_size_);
return (off1 ^ off2) < block_size_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

block_size_ here is equal to 0

block_bit_ += 1;
}
block_bit_ -= 1;
in_block_mask_ = (1ULL << block_bit_) - 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will we constrain the size of the block size to be a power of 2.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if not will round down

@LHT129 LHT129 force-pushed the improve branch 2 times, most recently from 58e0ad7 to c4d1b30 Compare December 13, 2024 04:20
Copy link
Collaborator

@inabao inabao left a comment

Choose a reason for hiding this comment

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

LGTM

if (io_param.contains(BLOCK_IO_BLOCK_SIZE_KEY)) {
this->block_size_ =
io_param[BLOCK_IO_BLOCK_SIZE_KEY]; // TODO(LHT): trans str to uint64_t
}
this->update_by_block_size();
Copy link
Collaborator

Choose a reason for hiding this comment

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

move to L47 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we have default block size value

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, but L44 will invoke update_by_block_size()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/io/memory_block_io.h Show resolved Hide resolved
@@ -109,14 +123,18 @@ class MemoryBlockIO : public BasicIO<MemoryBlockIO> {
Allocator* const allocator_{nullptr};

static const uint64_t DEFAULT_BLOCK_SIZE = 128 * 1024 * 1024; // 128MB

uint64_t block_bit_ = 27;
Copy link
Collaborator

Choose a reason for hiding this comment

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

block_bits = 27 corresponds to a DEFAULT_BLOCK_SIZE of 128MB. Should we also define a corresponding DEFAULT_BLOCK_BITS = 27 here ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -16,6 +16,7 @@
#include <immintrin.h>

#include <cmath>
#include <iostream>
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove unused header file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/simd/avx512.cpp Show resolved Hide resolved
- avx512 simd optimization
- memory_block_io optimization

Signed-off-by: LHT129 <[email protected]>
Copy link
Collaborator

@jiaweizone jiaweizone left a comment

Choose a reason for hiding this comment

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

LGTM

@LHT129 LHT129 merged commit 4845a3f into antgroup:main Dec 17, 2024
7 checks passed
@LHT129 LHT129 deleted the improve branch December 17, 2024 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants