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

filterx: perf improvement and fix race condition in readonly FilterXJson::to_json_literal() #258

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

MrAnno
Copy link
Member

@MrAnno MrAnno commented Aug 27, 2024

json_object_to_json_string_ext() writes/cached into the given JSON object, so it's not thread safe.

alltilla
alltilla previously approved these changes Aug 29, 2024
Copy link
Member

@alltilla alltilla left a comment

Choose a reason for hiding this comment

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

Nice catch, sorry for introducing this problem.

What would you think about making make_readonly() virtual and implementing the json specific parts there, so we don't need separate json_make_readonly() functions?

@MrAnno
Copy link
Member Author

MrAnno commented Aug 29, 2024

Much better, thanks.

@MrAnno
Copy link
Member Author

MrAnno commented Aug 29, 2024

The problem with this is that make_readonly() is transitive through get_subscript() and getattr() calls, and those are called from different threads during runtime.

So either we say we only let objects be marked readonly() at startup and we let go of the transitive behavior, or we keep everything but then I will have to either use locks or do a deep copy of the object (which defeats the whole idea of the read-only thing).

@alltilla
Copy link
Member

alltilla commented Sep 2, 2024

I think we could replace the make_readonly calls with an assert, and make sure to take care of it in the json and otel filterx object implementations. Maybe the assert could be in the list and dict interface getters.

The json object caches all the values, so we can do a deep traversal and make everything readonly when the root object becomes readonly, otel does not cache, it creates a new object, so we can must it readonly instantly.

Let's talk about this IRL.

Thanks for the investigation!

@MrAnno MrAnno changed the title filterx: fix race condition in readonly FilterXJson::to_json_literal() filterx: perf improvement and fix race condition in readonly FilterXJson::to_json_literal() Sep 2, 2024
@MrAnno MrAnno force-pushed the filterx-fix-readonly-json-str branch from 937fbf9 to 9165f96 Compare September 3, 2024 23:08
@MrAnno MrAnno requested a review from alltilla September 3, 2024 23:13
alltilla
alltilla previously approved these changes Sep 4, 2024
Copy link
Member

@alltilla alltilla left a comment

Choose a reason for hiding this comment

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

One minor suggestion, but it is completely okay for me to merge this as is. Let me know if you don't want to change the code based on my suggestion, and I will merge the PR.

Thanks!

lib/filterx/object-json-array.c Outdated Show resolved Hide resolved
json_object_to_json_string_ext() writes/caches into the given JSON object,
so it's not thread safe.

Signed-off-by: László Várady <[email protected]>
Copy link
Member

@alltilla alltilla left a comment

Choose a reason for hiding this comment

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

Awesome, this turned out really nice :)

@alltilla alltilla merged commit 48e1491 into axoflow:main Sep 5, 2024
22 checks passed
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