ibireme / yyjson

The fastest JSON library in C
https://ibireme.github.io/yyjson/doc/doxygen/html/
MIT License
2.98k stars 262 forks source link

Move ownership of a user allocated C string to yyjson object #139

Closed CarterLi closed 10 months ago

CarterLi commented 10 months ago

Is your feature request related to a problem? Please describe.

I have a lot of code like

void fillKey(yyjson_mut_doc* doc, yyjson_mut_val* obj, const char* key)
{
  char* str = getAllocatedStringResult(key);
  yyjson_mut_obj_add_strcpy(doc, obj, key, str);
  free(str);
}

We have yyjson_mut_x_add_str(n)cpy and yyjson_mut_x_add_str to add strings. The former api allocates and copies memories, which is sub-optimical. The latter api forces user maintaining the ownership of allocated memories, which is hard to use and generally limited to string literals.

Describe the solution you'd like

I'm propose a new api named yyjson_mut_x_add_str_moved(doc, x, str), which transfer the ownership of str to yyjson ( which means it will be freed when doc gets destroyed ). So that following code works

void fillKey(yyjson_mut_doc* doc, yyjson_mut_val* obj, const char* key)
{
  char* str = getAllocatedStringResult(key);
  yyjson_mut_obj_add_str_moved(doc, obj, key, str);
}

Describe alternatives you've considered

N/A

Additional context

N/A

ibireme commented 10 months ago

This needs to introduce a new pool within yyjson_mut_doc to manage these string pointers. I'll try to implement it later.

CarterLi commented 10 months ago

Can't we use the old pool? If implementing this feature requires introducing additional complexity, I prefer not implementing it.

ibireme commented 10 months ago

Can't we use the old pool? If implementing this feature requires introducing additional complexity, I prefer not implementing it.

The current string pool puts multiple copied strings in a large buffer, so they don't have to be freed one by one when destroyed.

If the strings are not copied, they need to be freed individually, so the existing pool will not work. What do you think?

CarterLi commented 10 months ago

So you are implementing your own malloc. Will the memory be reused if a string is removed from yyjson mut doc?

Please careful modifying public structs. It is a ABI imcompartible change.

ibireme commented 10 months ago

So you are implementing your own malloc. Will the memory be reused if a string is removed from yyjson mut doc?

No, the existing pool implementation is quite basic. It only grows and doesn't reuse, and it's mainly used to optimize building JSON.

Please careful modifying public structs. It is a ABI imcompartible change.

It seems to be more complex than I had initially anticipated. If you and others don't have a strong need for this feature, I believe it's best to hold off on making this change for now.

CarterLi commented 10 months ago

No, the existing pool implementation is quite basic. It only grows and doesn't reuse, and it's mainly used to optimize building JSON.

Most malloc implementations already use memory pool. You generally won't get much benefit by implementing your own.

It seems to be more complex than I had initially anticipated. If you and others don't have a strong need for this feature, I believe it's best to hold off on making this change for now.

Agreed.