stephenberry / glaze

Extremely fast, in memory, JSON and interface library for modern C++
MIT License
1.23k stars 123 forks source link

implement skip writing out default #1438

Open huangminghuang opened 1 week ago

huangminghuang commented 1 week ago

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

Note

This feature can be used to resolve IS #1410

stephenberry commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

stephenberry commented 1 week ago

@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 commented 1 week ago

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.

huangminghuang commented 1 week ago

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 commented 1 week ago

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