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] Replace std::cerr/cout with LOG and remove GLUTEN_PRINT_DEBUG macro #3171

Merged
merged 5 commits into from
Jan 5, 2024

Conversation

Yohahaha
Copy link
Contributor

@Yohahaha Yohahaha commented Sep 15, 2023

Replace all std::cerr/cout with LOG, replace DEBUG_OUT with DLOG, and remove GLUTEN_PRINT_DEBUG macro to avoid compilation issues caused by the lack of DEBUG testing.

@github-actions
Copy link

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:

@marin-ma
Copy link
Contributor

Thanks. Would you rebase?

@marin-ma
Copy link
Contributor

The find_package(glog REQUIRED) requires that the glog dependency is installed. Currently, glog is installed as part of building Velox dependencies. However, nowhere indicates that glog is a mandatory dependency for Gluten.

As it's moved to top-level CMakeList.txt, I think it would be better if we can have something like

  find_package(glog ${GLUTEN_GLOG_MIN_VERSION} CONFIG)
  if(NOT glog_FOUND)
    include(BuildGlog)
  endif()

@Yohahaha
Copy link
Contributor Author

yeah, I will do rebase later.

cpp/CMake/BuildGLog.cmake Outdated Show resolved Hide resolved
cpp/CMake/BuildGLog.cmake Outdated Show resolved Hide resolved
@Yohahaha
Copy link
Contributor Author

I dont know how to configure vcpkg correctly...

@marin-ma
Copy link
Contributor

cc: @zhouyuan

@zhouyuan
Copy link
Contributor

@Yohahaha @marin-ma here's the place to add glog dependency in vcpkg, If I understand this patch correctly
https://github.com/oap-project/gluten/blob/main/dev/vcpkg/vcpkg.json#L27-L42

thanks, -yuan

@marin-ma
Copy link
Contributor

marin-ma commented Nov 6, 2023

@Yohahaha I dig a bit deeper and found its probably caused by our Findglog.cmake module doesn't interface link to gflags. https://github.com/oap-project/gluten/blob/ba045c7705d8fe6cd2a52d7b303a6a01fd54a8d8/cpp/CMake/Findglog.cmake#L33-L37

Let me open another PR to solve it first.

@Yohahaha Yohahaha changed the title [CORE][VL] Add glog as top level dependency [VL] Replace std::cerr/cout with LOG Nov 9, 2023
cpp/core/CMakeLists.txt Outdated Show resolved Hide resolved
@Yohahaha
Copy link
Contributor Author

Yohahaha commented Nov 9, 2023

OpenJDK 64-Bit Server VM warning: You have loaded library /tmp/gluten-be155c79-5c02-4f41-85ae-0a8c3c520afa/jni/0b25eb42-3ef8-4f32-a4b7-5dad99d25da5/gluten-2591252663942864927/libvelox.so which might have disabled stack guard. The VM will try to fix the stack guard now.
It's highly recommended that you fix the library with 'execstack -c <libfile>', or link it with '-z noexecstack'.
ERROR: something wrong with flag 'timestamp_in_logfile_name' in file '/opt/gluten/dev/vcpkg/.vcpkg/buildtrees/glog/src/v0.6.0-1e4356b0ac.clean/src/logging.cc'.  One possibility: file '/opt/gluten/dev/vcpkg/.vcpkg/buildtrees/glog/src/v0.6.0-1e4356b0ac.clean/src/logging.cc' is being linked both statically and dynamically into this executable.

I will pending this path since it is not important, and vcpkg env confused me a lot...

Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale stale label Dec 26, 2023
@marin-ma
Copy link
Contributor

@Yohahaha This PR is very useful. Could you help to rebase?

@Yohahaha
Copy link
Contributor Author

@Yohahaha This PR is very useful. Could you help to rebase?

yeah, let me try again.

@Yohahaha
Copy link
Contributor Author

Yohahaha commented Dec 26, 2023

@marin-ma could you help remove stale label?

@Yohahaha Yohahaha changed the title [VL] Replace std::cerr/cout with LOG [VL] Replace std::cerr/cout with LOG and remove GLUTEN_PRINT_DEBUG macro Dec 26, 2023
@marin-ma marin-ma removed the stale stale label Dec 26, 2023
@@ -106,13 +105,11 @@ static inline jmethodID getStaticMethodIdOrError(JNIEnv* env, jclass thisClass,
static inline void attachCurrentThreadAsDaemonOrThrow(JavaVM* vm, JNIEnv** out) {
int getEnvStat = vm->GetEnv(reinterpret_cast<void**>(out), jniVersion);
if (getEnvStat == JNI_EDETACHED) {
DEBUG_OUT << "JNIEnv was not attached to current thread." << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not remove existing logs, unless for cleanup purposes. I would suggest replacing with DLOG(INFO).

@marin-ma
Copy link
Contributor

marin-ma commented Dec 28, 2023

Would you please include the replacement policies in the PR description? e.g. Replacing std::cerr with LOG(WARNING), replacing GLUTEN_PRINT_DEBUG with xxx, etc.

And please do not remove any existing debug logs. If cleanup or removal is necessary, let's address that separately in another PR.

Copy link
Contributor

@marin-ma marin-ma left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for tracking this PR! Just a few minor comments.

cpp/velox/jni/VeloxJniWrapper.cc Show resolved Hide resolved
cpp/core/utils/qpl/qpl_job_pool.cc Outdated Show resolved Hide resolved
cpp/core/utils/qpl/qpl_job_pool.cc Outdated Show resolved Hide resolved
@Yohahaha
Copy link
Contributor Author

Yohahaha commented Jan 4, 2024

@marin-ma @rui-mo please help review again, thanks!

@marin-ma
Copy link
Contributor

marin-ma commented Jan 5, 2024

LGTM. Thanks for iterating!

DEBUG_OUT << "Not found node id: " << nodeId << std::endl;
DEBUG_OUT << "Plan Node: " << std::endl << veloxPlan_->toString(true, true) << std::endl;
LOG(WARNING) << "Not found node id: " << nodeId;
LOG(WARNING) << "Plan Node: " << std::endl << veloxPlan_->toString(true, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

replace DEBUG_OUT with DLOG

Seems not matched with this description. Any reason here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the log type to warning in the exception block does make sense to me.

}
} catch (std::exception& e) {
DEBUG_OUT << __func__ << " call JavaInputStreamAdaptor::Close() got exception:" << e.what() << std::endl;
LOG(WARNING) << __func__ << " call JavaInputStreamAdaptor::Close() got exception:" << e.what();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not DLOG here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exception msg should be warning.

@rui-mo rui-mo merged commit 8c9b7d5 into apache:main Jan 5, 2024
17 checks passed
@GlutenPerfBot
Copy link
Contributor

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

query log/native_3171_time.csv log/native_master_01_04_2024_02823192e_time.csv difference percentage
q1 31.52 33.27 1.754 105.57%
q2 24.81 23.30 -1.507 93.93%
q3 38.53 39.36 0.825 102.14%
q4 39.17 38.81 -0.359 99.08%
q5 72.31 72.24 -0.079 99.89%
q6 7.03 7.06 0.027 100.38%
q7 87.37 87.34 -0.027 99.97%
q8 86.38 86.16 -0.219 99.75%
q9 126.50 126.86 0.357 100.28%
q10 44.86 45.86 1.002 102.23%
q11 20.15 20.86 0.705 103.50%
q12 24.99 26.68 1.694 106.78%
q13 45.96 46.62 0.662 101.44%
q14 18.17 19.84 1.670 109.19%
q15 29.10 29.67 0.570 101.96%
q16 16.37 15.55 -0.814 95.03%
q17 102.93 101.81 -1.123 98.91%
q18 150.99 152.15 1.157 100.77%
q19 12.80 12.61 -0.187 98.54%
q20 27.94 27.87 -0.066 99.76%
q21 225.09 227.23 2.146 100.95%
q22 14.09 14.02 -0.074 99.47%
total 1247.04 1255.16 8.113 100.65%

@Yohahaha Yohahaha deleted the core_glog branch January 5, 2024 04:23
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.

None yet

5 participants