Better API for writing widget data

[iacore]

Currently dvui.dataSet copies too much data.

I have the following code:

    fn widgetDo(this: *@This(), float: *dvui.FloatingWindowWidget, key: []const u8) !void {
        var code_buffer = dvui.dataGet(null, float.wd.id, key, [1024]u8) orelse .{0} ** 1024;
        defer dvui.dataSet(null, float.wd.id, key, code_buffer);

Here, 1024 bytes of data is copied per frame per widget.

I hope I can just get a slice to the memory location, then I can modify that directly.

[iacore]

Zig 0.11.0's support for local (filesystem) packages is garbage. The latest dev version fixed it, but dvui can't compile with it.

[david-vanderson]

I didn't think about using dataSet/dataGet for large data, but you can get what you want. This example only calls dataSet() the first time. dataGet() will return a slice pointing to the internal data if given a slice type.

var hbox = try dvui.box(@src(), .horizontal, .{});
defer hbox.deinit();
var buf: []u8 = undefined;
if (dvui.dataGet(null, hbox.wd.id, "data_key", []u8)) |data| {
    buf = data;
} else {
    var stack_buf: [1024]u8 = undefined;
    @memset(&stack_buf, 0);
    buf = &stack_buf;
    dvui.dataSet(null, hbox.wd.id, "data_key", buf);
    buf = dvui.dataGet(null, hbox.wd.id, "data_key", []u8).?;
}
var te = try dvui.textEntry(@src(), .{
    .text = buf,
}, .{});
te.deinit();

For the other question, should we again follow zig master? I figured using a stable zig would be easier, but maybe everybody trying dvui right now is happy to keep up with the breakage?

[iacore]

For the other question, should we again follow zig master? I figured using a stable zig would be easier, but maybe everybody trying dvui right now is happy to keep up with the breakage?

Zig master is breaking every day; not worth it in my opinion. I found a way to just edit content inside ~/.cache/zig/p/XXXXXXXX.

[iacore]

This line, the lifetime seems weird. buf/stack_buf lives on stack, which I don't want.

    dvui.dataSet(null, hbox.wd.id, "data_key", buf);

[david-vanderson]

Good to know about zig master. We'll stay on 0.11.x for now.

Options for the textEntry buffer if you don't want to manage the storage yourself:

1. We could add to the dataGet() api a default value. It would accomplish the same end as the example above, but maybe more obvious what's going on (the stack value was just to provide a default value that gets copied into the data store). This sounds like a good idea in any case.

2. Maybe you'd like the textEntry to allocate and manage the storage internally? We could do this, but it would be complicated especially if you want the storage to grow/shrink. Then there's also the question of how to get the data out. Either we specify how to use dataGet to get it, or you have to only use it between the install()/deinit() of the widget?

Does 1 work for you?

2 seems like maybe we'll need that eventually but I'm less sure how useful it'd be without a clear use-case to drive the design.

[iacore]

Options for the textEntry buffer if you don't want to manage the storage yourself:

I think you have misunderstood me.

I was confused because the behavior of the following two signatures are not the same.

dvui.dataSet(_, _, _, []u8); // copy the slice content (external to []u8)

dvui.dataSet(_, _, _, struct{}); // copy the data (only struct{})

In Zig, []u8 only contains ptr and len.

[david-vanderson]

Yep - sorry totally misunderstood. You are right that the behavior is a bit weird. It seemed okay when I wrote it, but it needs to be clearer. Thanks for pointing it out!

How about we make dataSet() always copy only the given data, and add a dataSetSlice() (and dataGetSlice()) for the other case?

[iacore]

How about we make dataSet() always copy only the given data, and add a dataSetSlice() (and dataGetSlice()) for the other case?

This is ok.

I think a better design is to have the API tied up as dataCursor(_, _, _, _).xxxx(), so that no multiple lookups are done.

There's also the hashmap api

const entry = dvui.dataGetOrPut(..., [1024]u8);
if (entry.exists) { ... } else {
    entry.value_ptr.* = ...;
}

[david-vanderson]

I've committed what I hope is a better api for this. I spent a lot of time playing around with different versions of it.

dvui.dataSet() will copy slice content into internal storage by default, because I found that is almost always what you want. dvui.dataGet() on a slice returns a slice of the internal storage, so there is no copy on that side.

dvui.dataSetAdvanced() gives control over whether the slice content is copied.

dvui.dataGetPtr() returns a pointer to the internal storage which can avoid copying a large struct.

dvui.dataGetDefault() sets a value if not found and returns it, similar for dataGetPtrDefault()

Does this work for you? Also I was trying to think of a time when you would want to store only the ptr+len of a slice, and not the contents, but am having trouble. Do you have a use-case in mind for that?

[iacore]

dvui.dataSet() will copy slice content into internal storage by default, because I found that is almost always what you want.

I still don't like this. Maybe the data is managed externally.

dvui.dataSetAdvanced() gives control over whether the slice content is copied.

I think copy_slice=false should be the default and only behavior.
Also this still copies data.

dvui.dataGetPtr() returns a pointer to the internal storage which can avoid copying a large struct.

So dataGet() is not needed.

dvui.dataGetDefault() sets a value if not found and returns it, similar for dataGetPtrDefault()

I don't know if this is necessary part of the API, since it uses dataGet and dataSet.

Does this work for you? Also I was trying to think of a time when you would want to store only the ptr+len of a slice, and not the contents, but am having trouble. Do you have a use-case in mind for that?

No.

I have wrote an application, where I fetch JSON from network, and decode it. The result object is managed by libgc (bohem GC).

I don't want to copy the strings since it's not necessary.

[iacore]

I still think that Zig's HashMap API is good.

• put(T) !void
• put(T) !?T: this variant returns previous value
• getPtr(@typeof(T)) ?*T
• getOrPutPtr(@typeof(T)) !*T: allocates memory, fill with undefined, returns pointer

Why fill with undefined and return pointer: the "default" value may need some calculation; we should only calculate the value when it is needed.

Of course, the user can pass in undefined as the default value for any type too.

[david-vanderson]

I think copy_slice=false should be the default and only behavior.
Also this still copies data.

I'm confused by this. What data is being copied that you don't want? If you pass a slice to dataSetAdvanced with copy_slice false, then only the ptr+len is being stored. dataGetPtr will return a pointer to that ptr+len. Is that not what you are seeing?

The problem with copying the hashmap api is precisely that many times I've wanted widgets to store short strings. Like a dialog will store the title and message, which are cleaned up once the dialog stops being shown. When I tried having a slice-specific version, then it was easy to use the regular one and end up with a slice to temporary storage.

You are right that dataGet and the default is not strictly necessary. But dataGet is very useful for the common case of a widget wanting to store a bool, or a Rect. Any code that is using more of an immutable style.

You are also right the Default variants aren't necessary either. Probably should remove them and live with that for a while.

[iacore]

The problem about copying data is another small optimization issue. a different problem.

Let's say you have some data already. dataSet copies that data into internal hashmap. (compiler can't optimize this well)

In some cases, you don't have the data already. You want dvui to give you a location (pointer) to put those data. There's no copy here because you generate the data at the target location.

[iacore]

The problem with copying the hashmap api is precisely that many times I've wanted widgets to store short strings. Like a dialog will store the title and message, which are cleaned up once the dialog stops being shown. When I tried having a slice-specific version, then it was easy to use the regular one and end up with a slice to temporary storage.

Internal API doesn't have to be the same as public API. You can preserve the current dataSet behavior but make it private. That's what I think.

[david-vanderson]

In some cases, you don't have the data already. You want dvui to give you a location (pointer) to put those data. There's no copy here because you generate the data at the target location.

Yes - thank you for the example of the JSON - that motivates wanting to store a slice but not the contents.

I think we both have reasonable but different ideas of what the more common use cases are vs. less common. Does the api now at least support what you want to do?

If it does, why don't we live with it for a while and revisit it with more experience at some point?

[iacore]

My opinion is that API should be consistent. For generics, it means doing the same thing for every type in Zig. It means no surprises if people don't read the docs carefully.

If it does, why don't we live with it for a while and revisit it with more experience at some point?

If you say so.

Can you explain clearly in the comments what is a "slice"?

Currently it wrote:

/// If T is a slice, returns slice of internal storage, which will be freed

Zig has so many slices... *[_]T, []T, [:0]T, [_]T

[david-vanderson]

I've tried to make it better by listing the two possibilities ([]T and [:X]T). Hopefully that helps distinguish. I think we should be okay regarding *[_]T and [_]T because those are compile errors without being followed by an array literal.

Thanks for listing the sentinel slice, because those were broken. I've committed code to support them.

I will look into whether zig's std.mem.sliceAsBytes should include the sentinel value. It doesn't right now, and I'm pretty sure it should.

[iacore]

I've tried to make it better by listing the two possibilities ([]T and [:X]T). Hopefully that helps distinguish. I think we should be okay regarding *[_]T and [_]T because those are compile errors without being followed by an array literal.

[]struct {} is copied too?

Thanks for listing the sentinel slice, because those were broken. I've committed code to support them.

maybe only support []u8 and [:x]u8 is good.

It still feels weird to copy slice content, since slices in Zig are not owning.

[david-vanderson]

You convinced me to try it the explicit way. Thanks for being persistent!

I changed it back so that your way is the default, and to copy the slice contents you call dataSetSlice/dataGetSlice. We'll try that for a while and see if it trips up anybody. I'm primarily worried about people doing dataSet(null, id, "key", "temporary string") and then getting memory corruption later. But we'll see what happens.

Thanks very much!