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

implement skip writing out default #1438

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

huangminghuang
Copy link
Contributor

Motivation

The primary goal of this feature is to mimic protobuf behavior, where fields with default values are omitted from the wire format. Additionally, a new printout option allows users to override this behavior if they wish to include fields with default values. To support this flexibility, the skip_null_members flag in Opts has been redefined as a bitmask, allowing independent control over skipping null and default values.

Key Changes

  • Conditional Skipping with custom Wrapper: The custom wrapper now enables users to define a function that determines whether a field should be omitted based on specific conditions. An associated bitmask has been introduced to allow compile-time checks, which control whether user-defined functions should be evaluated based on the Opts::skip_null_members setting during serialization.
  • New skip_write_default Wrapper: A skip_write_default wrapper provides a concise way to skip fields with default values, streamlining the setup for this common use case.

Note

This feature can be used to resolve IS #1410

@stephenberry
Copy link
Owner

This is awesome! Thanks for tackling this desired feature of skipping default values. I look forward to merging this, but I want to walk through the code changes carefully first. But, your changes look great!

I don't think I want to make a breaking change at this point (so I appreciate your approach), but I'll open a discussion for Glaze v5 for breaking changes that would be helpful for clarity.

@huangminghuang
Copy link
Contributor Author

I am glad you like it. However, from the github action run , there is a race condition in repe_test which I don't think is introduced by my change. You may need to tackle it.

@stephenberry
Copy link
Owner

stephenberry commented Nov 12, 2024

@huangminghuang, yes I saw that race condition recently and need to tackle it. It's not from your code.

@stephenberry
Copy link
Owner

@huangminghuang, I just made a change to your pull request that allocates a static thread_local instance of the default class. Your code would allocate and then destroy an instance every time a variable was checked to see if it was the default. This would be extremely poor performing for heap allocated structures.

Now, we should be able to avoid the thread_local instance and instead make a static constexpr instance for types that are constexpr default constructible. But, I was having issues writing a sufficient concept that would reject a class that contains a std::string. Because, in C++20 std::string's constructor is constexpr qualified, it is just that the heap allocated memory must be destroyed before runtime. If you can figure out a solution that would be great, but the static thread_local variable works for now.

@stephenberry
Copy link
Owner

One other aspect of this pull request that we might want to change is the behavior of the skip_null_members options when the skip_default_flag is set. Currently in Glaze the skip_null_members option is globally applied, which means that you don't have to use glz::custom to get the behavior and you can use it with reflected types that have no glz::meta. I feel like we want the skip_default_flag to work the same way, so as to not require a glz::meta nor require glz::custom. I do like the fine grained approach of being able to customize the behavior for each field, but often this is desired as a global effect for the glz::write call.

This requires some more work, but I think it will be worth it in the long run. If you don't feel comfortable tackling this, I can work on it when I have time. But, it would be great if you were able to mimic the behavior of the other options and get global behavior working.

struct deduct_default_t
{};

template <auto MemPtr, auto Default, class T>
bool is_default(const T& val)
{
if constexpr (std::same_as<std::decay_t<decltype(Default)>, deduct_default_t>) {
return val.*MemPtr == T{}.*MemPtr;
static thread_local auto defaulted = T{};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the thread_local is unnecessary because C++ requires static variable initialization to be thread safe since C++11. The statement might be better as follows.

static const auto defaulted = T{}.*MemPtr;
return val.*MemPtr == defaulted;

I expected the compiler can be smart enough not to construct unused data members this way; however, based on my experiment on compiler explorer, no compiler can do that.

Copy link
Owner

Choose a reason for hiding this comment

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

You are correct, because we aren't changing state, we can have a single, global static variable. I'm just used to requiring thread_local for mutation. We can also make it const.

@huangminghuang
Copy link
Contributor Author

One other aspect of this pull request that we might want to change is the behavior of the skip_null_members options when the skip_default_flag is set. Currently in Glaze the skip_null_members option is globally applied, which means that you don't have to use glz::custom to get the behavior and you can use it with reflected types that have no glz::meta. I feel like we want the skip_default_flag to work the same way, so as to not require a glz::meta nor require glz::custom. I do like the fine grained approach of being able to customize the behavior for each field, but often this is desired as a global effect for the glz::write call.

I think we should consider the semantic carefully about this. The current design is to use glz::custom to declare a field has a default value, and use the glz::opts to control whether we should skip the default. If I understand you right, you want that every field has a default value implicitly and can be skipped using skip_default_flag. In the case, you may want a way to declare a field cannot be skipped anyway; use glz::custom with skip_mask=0 may work. However, I feel we need two different flags: skip_custom_default_flag and skip_implicit_default_flag to differentiate between the two scenarios.

@stephenberry
Copy link
Owner

I agree, any general option for skipping defaults would need an opt-out option, like you've noted with glz::custom.

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