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

adds jv_unshare(), jv_is_unshared() #3109

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/jq_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -490,4 +490,19 @@ static void jv_test() {
//jv_dump(jv_copy(o2), 0); printf("\n");
jv_free(o2);
}

{
jv test_unshared = JV_OBJECT(jv_string("some"), JV_ARRAY(jv_string("other"), jv_true()));

assert(jv_is_unshared(test_unshared));
jv_free(test_unshared);

jv initial = jv_parse("{\"test\":[{\"some\":\"value\"}, 1, true, false, null]}");
jv new = jv_unshare(jv_copy(initial));

assert(jv_equal(jv_copy(initial), jv_copy(new)));
assert(jv_is_unshared(new));
jv_free(initial);
jv_free(new);
}
}
126 changes: 126 additions & 0 deletions src/jv.c
Original file line number Diff line number Diff line change
Expand Up @@ -1862,6 +1862,132 @@ jv jv_object_iter_value(jv object, int iter) {
/*
* Memory management
*/
jv jv_unshare(jv input){
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how ugly but i guess one could save on jv_free calls by having different ownership rule for unshare? too big foot gun?

Copy link
Author

Choose a reason for hiding this comment

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

even within the implementation of jv_unshare() we benefit from it dereferencing its input. We could have jv_unshare() not consume its memory, but at the cost of having to write a lot of stuff to buffers

Copy link
Contributor

Choose a reason for hiding this comment

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

Is jv_unshare() supposed to consume a reference to its argument? I think it should.

switch(jv_get_kind(input)){
case JV_KIND_INVALID:
{
if(!jv_invalid_has_msg(jv_copy(input))){
jv_free(input);
return jv_invalid();
}
return jv_invalid_with_msg(jv_unshare(jv_invalid_get_msg(jv_copy(input))));
}
case JV_KIND_OBJECT:
{
jv keys = jv_keys(jv_copy(input));
size_t keys_length = jv_array_length(jv_copy(keys));

jv output_object = jv_object();

for(size_t i = 0; i < keys_length; i++){
jv key = jv_array_get(jv_copy(keys), i);
output_object = jv_object_set(
output_object, jv_unshare(key),
jv_unshare(
jv_object_get(
jv_copy(input),
jv_copy(key)
)
)
);
}

jv_free(keys);
jv_free(input);
return output_object;
}
case JV_KIND_ARRAY:
{
size_t amount = jv_array_length(jv_copy(input));

jv output_array = jv_array_sized(amount);

for(size_t i = 0; i < amount; i++){
output_array = jv_array_set(
output_array,
i,
jv_unshare(
jv_array_get(
jv_copy(input),
i
)
)
);
}

jv_free(input);

return output_array;
}
case JV_KIND_STRING:
{
jv output_string = jv_string(jv_string_value(input));
jv_free(input);
return output_string;
}
case JV_KIND_NUMBER:
{
double val = jv_number_value(input);
jv_free(input);
return jv_number(val);
}
case JV_KIND_TRUE:
jv_free(input);
return jv_true();
case JV_KIND_FALSE:
jv_free(input);
return jv_false();
case JV_KIND_NULL:
jv_free(input);
return jv_null();
default:
return jv_invalid();
}
}

int jv_is_unshared(jv a){
if(jv_get_refcnt(a) != 1){
fprintf(stderr, "input refcnt != 1\n");
return 1;
}
if(jv_get_kind(a) == JV_KIND_OBJECT){
jv keys = jv_keys(jv_copy(a));
Copy link
Member

Choose a reason for hiding this comment

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

I don't know much about the jv internals but i suspect that maybe jv_is_unshared is suitable to be implemented using a private new jvp_* helper function that uses some internal details, similar to how jvp_object_equal etc is implemented? then you can probably save on some allocation and refcounting.

@nicowilliams does that make sense?

Copy link
Author

Choose a reason for hiding this comment

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

sorry for my inactivity for the past month on the thread. While having a jvp internal representation would get rid of some of the ref-counting it will add more complexity. Then again if you think it's worth I could implement jvp_object_is_unshared(), jvp_array_is_unshared() (I would refrain from adding jvp functions for the non-recursive options as I don't think there is much benefit to be had in these senarios)

Or do you think I should add string, number, true, false, null, invalid variants as well?

size_t keys_length = jv_array_length(jv_copy(keys));
for(size_t i = 0; i < keys_length; i++){
jv key = jv_array_get(jv_copy(keys), i);
if(jv_get_refcnt(key) > 3){
fprintf(stderr, "key in object does not have refcnt 1, %d\n", jv_get_refcnt(key));
jv_free(key);
jv_free(keys);
return 0;
}

jv value = jv_object_get(jv_copy(a), key);
jv_free(value);
if(!jv_is_unshared(value)){
fprintf(stderr, "failed on object\n");
jv_free(keys);
return 0;
}
}

jv_free(keys);

}else if(jv_get_kind(a) == JV_KIND_ARRAY){
size_t a_length = jv_array_length(jv_copy(a));
for(size_t i = 0; i < a_length; i++){
jv value = jv_array_get(jv_copy(a), i);
jv_free(value);
if(!jv_is_unshared(value)){
fprintf(stderr, "failed on array value\n");
return 0;
}
}
}

return 1;
}

jv jv_copy(jv j) {
if (JVP_IS_ALLOCATED(j)) {
jvp_refcnt_inc(j.u.ptr);
Expand Down
3 changes: 3 additions & 0 deletions src/jv.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ jv_kind jv_get_kind(jv);
const char* jv_kind_name(jv_kind);
static int jv_is_valid(jv x) { return jv_get_kind(x) != JV_KIND_INVALID; }

//jv_unshare() creates a deep copy of the input aka the content of the output will be identical to the input, but no shared memory exists between them
jv jv_unshare(jv);
int jv_is_unshared(jv);
jv jv_copy(jv);
void jv_free(jv);

Expand Down
3 changes: 3 additions & 0 deletions src/jv_aux.c
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,8 @@ jv jv_setpath(jv root, jv path, jv value) {
return jv_set(root, pathcurr, jv_setpath(subroot, pathrest, value));
}



jv jv_getpath(jv root, jv path) {
if (jv_get_kind(path) != JV_KIND_ARRAY) {
jv_free(root);
Expand Down Expand Up @@ -538,6 +540,7 @@ static int string_cmp(const void* pa, const void* pb){
return r;
}


jv jv_keys_unsorted(jv x) {
if (jv_get_kind(x) != JV_KIND_OBJECT)
return jv_keys(x);
Expand Down
Loading