Skip to content

[GLUTEN-2974] [VL] Minor refactor for AllocationListener and VeloxMemoryMamanger to non copyable and non assignable - #2916

Merged
zhztheplayer merged 1 commit into
apache:mainfrom
Yohahaha:reservation_listener
Sep 7, 2023
Merged

zhztheplayer merged 1 commit into
apache:mainfrom
Yohahaha:reservation_listener

Conversation

@Yohahaha

@Yohahaha Yohahaha commented Aug 28, 2023 •

Copy link
Copy Markdown
Contributor

Fixes: #2974

@github-actions

Copy link
Copy Markdown

Thanks for opening a pull request!

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

https://github.com/oap-project/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:

@Yohahaha
Yohahaha force-pushed the reservation_listener branch from e96c83e to 9414bb8 Compare August 28, 2023 02:59
@Yohahaha Yohahaha changed the title [VL] Minor refactor of rename AllocationListener to ReservationListener [VL] Minor refactor for rename AllocationListener to ReservationListener Aug 28, 2023
Comment thread cpp/core/memory/ReservationListener.h Outdated
Comment thread cpp/core/memory/MemoryAllocator.cc Outdated
Comment thread cpp/velox/memory/VeloxMemoryManager.h Outdated
@Yohahaha
Yohahaha force-pushed the reservation_listener branch from 9dd3473 to 8a09348 Compare August 28, 2023 06:27
@Yohahaha Yohahaha changed the title [VL] Minor refactor for rename AllocationListener to ReservationListener [VL] Minor refactor for AllocationListener and VeloxMemoryMamanger to non copyable and non assignable Aug 28, 2023
@Yohahaha
Yohahaha force-pushed the reservation_listener branch from 8a09348 to 0b44412 Compare August 28, 2023 09:14
@Yohahaha
Yohahaha requested a review from zhztheplayer August 29, 2023 04:20
@zhztheplayer

Copy link
Copy Markdown
Member

@Yohahaha Would you like to do rebase? Thanks!

@Yohahaha
Yohahaha force-pushed the reservation_listener branch from 0b44412 to 4cf9c1f Compare August 30, 2023 13:30
@Yohahaha Yohahaha changed the title [VL] Minor refactor for AllocationListener and VeloxMemoryMamanger to non copyable and non assignable [GLUTEN-2974] [VL] Minor refactor for AllocationListener and VeloxMemoryMamanger to non copyable and non assignable Aug 30, 2023
@github-actions

Copy link
Copy Markdown

#2974

Comment thread cpp/core/jni/JniCommon.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: this is not necessarily a local ref. Caller could pass a global ref in if wanted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We should limit usage, create listener from java call native, and pass local ref in, only convert to global ref in listener internal, let listener manage this ref, otherwise, we can not manage this ref easily.

I'm not sure whether newGlobalRef is idempotent...

Comment thread cpp/core/jni/JniCommon.h Outdated
@zhztheplayer

Copy link
Copy Markdown
Member

Would you like to rebase ? Thanks! @Yohahaha

fix

fix
@Yohahaha
Yohahaha force-pushed the reservation_listener branch from 4cf9c1f to 5353239 Compare September 6, 2023 02:21
@Yohahaha

Yohahaha commented Sep 6, 2023

Copy link
Copy Markdown
Contributor Author

@zhztheplayer already rebased, but here has a network issue in CI, could you help re-run failed jobs?

@zhztheplayer

Copy link
Copy Markdown
Member

@rui-mo Would you like to take a look?

@zhztheplayer
zhztheplayer merged commit 172820c into apache:main Sep 7, 2023
@Yohahaha
Yohahaha deleted the reservation_listener branch September 7, 2023 06:04
@GlutenPerfBot

Copy link
Copy Markdown
Contributor

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

query log/native_2916_time.csv log/native_master_09_06_2023_fb7f707e7_time.csv difference percentage
q1 44.55 43.16 -1.385 96.89%
q2 24.66 24.64 -0.027 99.89%
q3 36.48 37.33 0.854 102.34%
q4 42.04 41.42 -0.616 98.54%
q5 69.51 70.22 0.711 101.02%
q6 6.34 5.16 -1.184 81.34%
q7 85.08 84.66 -0.418 99.51%
q8 81.80 81.38 -0.420 99.49%
q9 117.65 114.35 -3.300 97.20%
q10 48.55 48.41 -0.138 99.72%
q11 19.52 19.58 0.056 100.29%
q12 25.38 24.98 -0.392 98.46%
q13 51.12 49.53 -1.584 96.90%
q14 16.75 16.77 0.023 100.14%
q15 30.09 30.10 0.004 100.01%
q16 15.46 16.20 0.746 104.83%
q17 121.37 120.43 -0.934 99.23%
q18 162.38 164.70 2.325 101.43%
q19 12.45 12.30 -0.148 98.81%
q20 32.18 31.16 -1.024 96.82%
q21 245.28 238.90 -6.377 97.40%
q22 16.22 15.29 -0.934 94.24%
total 1304.86 1290.69 -14.162 98.91%

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.

[VL] Let VeloxMemoryManager manage AllocationListenr and be non copyable/assignable

3 participants