-
Notifications
You must be signed in to change notification settings - Fork 849
Reduce overhead incurred by enabling debug output. #8581
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /** @file | ||
| DbgCtl class header file. | ||
| @section license License | ||
| Licensed to the Apache Software Foundation (ASF) under one | ||
| or more contributor license agreements. See the NOTICE file | ||
| distributed with this work for additional information | ||
| regarding copyright ownership. The ASF licenses this file | ||
| to you under the Apache License, Version 2.0 (the | ||
| "License"); you may not use this file except in compliance | ||
| with the License. You may obtain a copy of the License at | ||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #include <ts/ts.h> | ||
|
|
||
| // For use with TSDbg(). | ||
| // | ||
| class DbgCtl | ||
| { | ||
| public: | ||
| // Tag is a debug tag. Debug output associated with this control will be output when debug output | ||
| // is enabled globally, and the tag matches the configured debug tag regular expression. | ||
| // | ||
| DbgCtl(char const *tag) : _ptr(_get_ptr(tag)) {} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean checking that tag is not null? I don't think we normally call TS API functions from core TS. |
||
|
|
||
| TSDbgCtl const * | ||
| ptr() const | ||
| { | ||
| return _ptr; | ||
| } | ||
|
|
||
| // Call this when the compiled regex to enable tags may have changed. | ||
| // | ||
| static void update(); | ||
|
|
||
| private: | ||
| TSDbgCtl const *const _ptr; | ||
|
|
||
| static const TSDbgCtl *_get_ptr(char const *tag); | ||
|
|
||
| class _RegistryAccessor; | ||
|
|
||
| friend TSDbgCtl const *TSDbgCtlCreate(char const *tag); | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way this volatile declaration is used is undefined behavior. See usage for volatile after C++11. particularly:
This makes volatile objects suitable for communication with a signal handler, but not with another thread of execution, see std::memory_order). Any attempt to refer to a volatile object through a glvalue of non-volatile type (e.g. through a reference or pointer to non-volatile type) results in undefined behavior.
If you want to guarantee that changes are seen immediately in another thread, you will need to use some memory ordering mechanism. Otherwise, other threads will see changes the next time that memory is read.
I tested this without volatile and it still adjusts the debug that are printed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reality is that ATS often relies on de facto portability, rather than portability guaranteed by the Standard.
I guess memory ordering has been added to straight C, but I don't know the specifics, I'll have to research this. The point of the volatile is to emphasize that that the on flag should not be register cached. But it seems implausible the compiler could do that indefinitely anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's correct that (when using Debug() instead of Dbg() ) there's a significant increase in loaded data segment size, But, it's a delta on top of the high resource use of trace output even with the old design. Which seems worthwhile, to make tracing usable in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is a worthwhile trade off for performant debugging! It's good stuff.
I also think that in this case, any changes to
onvalues will be seen "soon enough" by other threads and that volatile just isn't necessary here(especially since its accessed through a non-volatile pointer).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it's unlikely, this code illustrates the sort of optimization that could happen to a Debug statement in a loop, if we don't do something to prevent the compiler from register-caching the 'on' flag: https://godbolt.org/z/r4K9TP1qW
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternate to simply relying on volatile would be to use atomic_load_explicit() with relaxed memory order. But that would mean we'd have to require plugins to be build with C11 support, which I don't think we've done up till now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the Standard says that the behavior of reinterpret_cast is undefined, but ATS uses it over 1,400 times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is closer to (but still not quite) the situation we have here. We are accessing a volatile through a (non-volatile) static pointer. The compiler must read from memory at least one time when the containing function is called. For the non-volatile case, the compiler can then assume the value won't change until another function is called or the original function is called again. This is what I meant by it will be re-read "soon enough".
All that said, if you are happier with the volatile qualifier and no-one else objects, I'm not overly concerned about it.