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

[VL] Following #6526, minor fixes and improvements #6554

Merged
merged 3 commits into from
Jul 24, 2024

Conversation

zhztheplayer
Copy link
Member

@zhztheplayer zhztheplayer commented Jul 23, 2024

A follow-up to #6526

  1. Use the old way before [VL] Move memory reservation block computation logic into ListenableAllocator #5770 for block reservation because
    • It uses same algorithm for diff < 0 and diff > 0 so simplifies code
    • It reduces code branches
  2. Code peakBytes_.store(std::max(peakBytes_.load(), usedBytes_.load())) is still not atomic so needs to be locked

@apache apache deleted a comment from github-actions bot Jul 23, 2024
Copy link

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

@@ -87,11 +73,28 @@ class BlockAllocationListener final : public AllocationListener {
}

private:
int64_t reserve(int64_t diff) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add inline

if (peakBytes_.compare_exchange_weak(savedPeakBytes, usedBytes_)) {
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

With C++26 in future, we can use fetch_max directly. https://en.cppreference.com/w/cpp/atomic/atomic/fetch_max

if (peakBytes_.compare_exchange_weak(savedPeakBytes, usedBytes_)) {
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

With C++26 in future, we can use fetch_max directly. https://en.cppreference.com/w/cpp/atomic/atomic/fetch_max

@zhztheplayer zhztheplayer merged commit 33d2f2d into apache:main Jul 24, 2024
41 checks passed
weiting-chen pushed a commit to weiting-chen/gluten that referenced this pull request Jul 25, 2024
weiting-chen pushed a commit to weiting-chen/gluten that referenced this pull request Jul 25, 2024
weiting-chen added a commit that referenced this pull request Jul 25, 2024
* [GLUTEN-6501][VL] Fix the missing fileReadProperties when constructing a LocalFilesNode (#6503)

* [GLUTEN-6477][VL] Fix occasional dead lock during spilling (#6515)

* [VL] Add thread_safe to several VeloxRuntime classes (#6526)

VeloxRuntime is shared by many threads, like task threads or parquet writter threads. We must make sure the objects hold by VeloxRuntime are thread safe.

* [VL] Following #6526, minor fixes and improvements (#6554)

---------

Co-authored-by: zhaokuo <[email protected]>
Co-authored-by: Hongze Zhang <[email protected]>
Co-authored-by: BInwei Yang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants