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

Introduce logging levels for conditional logging and log key for NO_SUCH_FIELD error #2004

Merged
merged 12 commits into from May 22, 2023
Merged

Introduce logging levels for conditional logging and log key for NO_SUCH_FIELD error #2004

merged 12 commits into from May 22, 2023

Conversation

yongxiangng
Copy link
Contributor

This resolves #2003

SIMDJSON_LOG_LEVEL Environment variable

Introduces an environment variable SIMDJSON_LOG_LEVEL which is available when compiled with the compile option SIMDJSON_VERBOSE_LOGGING

SIMDJSON_LOG_LEVEL can either be INFO or ERROR

Key not found logging

Added logging for key when NO_SUCH_FIELD error occurs.

@lemire
Copy link
Member

lemire commented May 15, 2023

Looks reasonable.

@lemire
Copy link
Member

lemire commented May 16, 2023

Running tests.

@lemire
Copy link
Member

lemire commented May 22, 2023

Merging.

@lemire
Copy link
Member

lemire commented Jul 7, 2023

@yongxiangng

This has a significant negative performance impact for various reasons:

  1. Logging overhead went being null unless a compile-time constant is set to being non-null a runtime constant. This means that we can get the cost of logging (and not "for free") even if logging is disabled.
  2. Temporary small strings are created when accessing keys in objects.

I have to remove it. I do invite you to produce a new PR with the same functionality but it should be free.

cc @jkeiser

@lemire
Copy link
Member

lemire commented Jul 7, 2023

@yongxiangng

Let me give an example... Compilers will compile match to the empty function:

void log(const char* x) {}


void match(std::string_view v) {
    log("t:");
}

What you PR does is ask too much of the compiler... You do the equivalent to ...

void log(const char* x) {}

void match(std::string_view v) {
    std::string s = "t:";
    s += v;
    log(s.c_str());
}

What this does is not at all trivial. The compiler creates an std::string, calls the constructor then call the destructor. Yes, I suppose that in theory the compiler could just wipe this out, but it won't.

IMPORTANTLY: You pay the price even do the log function is empty!

So this won't do.

We need a better solution.

@yongxiangng
Copy link
Contributor Author

@lemire I'm very sorry about that. Yes, I did assume the compiler would have optimized it away, because the log function might get inlined, and the log body is empty when logging is off (also increases chance of inlining), hence the compiler might be able to deduce msg isn't being used and also optimize that away. But I do agree that's not trivial for the compiler and the evidence proves that there is a performance penalty. I am sorry.

I did think of trying to do lazy evaluation, e.g.

template <typename... Args>
void log(std::string_view format_str, Args&&... args)
{
    if (logging_enabled)
    {
        // do the string formatting inside here
    }
}

but the current signature doesn't allow for that because of the defaulted arguments at the back

static inline void log_error(const json_iterator &iter, const char *error, const char *detail="", int delta=-1, int depth_delta=0) noexcept;

I can't think of a good way to solve this problem for now

Thanks for reverting the PR given the severe performance impact and I apologize for the trouble caused.

@lemire
Copy link
Member

lemire commented Jul 7, 2023

@yongxiangng No apology needed. I was not criticizing you. I am the one who made the mistake.

It is actually doable, logging without overhead.

The following should work...

https://github.com/ada-url/ada/blob/main/include/ada/log.h

Can you have a look?

I do want to merge your solution, we just want it to be free when logging is disabled.

@lemire
Copy link
Member

lemire commented Jul 7, 2023

Also, we found out that LOG_ERROR and LOG_INFO were not good choices because people use them as macros (I did not know that).

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.

Enable error logging
2 participants