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

Add a way to return an empty resultset without using G_IO_ERROR_NOT_FOUND #149

Open
pwithnall opened this issue Sep 8, 2023 · 7 comments

Comments

@pwithnall
Copy link
Collaborator

Currently, libxmlb queries will set the error G_IO_ERROR_NOT_FOUND if they find no matching elements. A lot of queries will often return no results (that’s just the way XML is), so this code path is hit often. If a lot of queries are run quickly (for example, when loading gnome-software), the cost of allocating a GError to indicate an empty resultset becomes measurable.

It would be nice if libxmlb would provide an alternate codepath which indicates an empty resultset some other way, without needing an allocation.

This might be possible without any new API: the documentation for the xb_silo_query() functions already says they return NULL if unfound, so callers should already be handling that. If that’s acceptable, we could just basically remove the g_set_error (G_IO_ERROR_NOT_FOUND) calls.

See https://gitlab.gnome.org/GNOME/gnome-software/-/merge_requests/1749 for some more context.

gnomesysadmins pushed a commit to GNOME/gnome-software that referenced this issue Sep 8, 2023
Providing a GError to libxmlb results in the XbMachine internals
propagating errors to the caller beyond the basic G_IO_ERROR_NOT_FOUND.

This is not ideal because it means that a lot of CPU time is spent on
creating and freeing GErrors in tight loops that is otherwise
non-essential for control flow.

Another option would be to add API to libxmlb to denote the difference
between a result set, empty set (not found), and error but that will
require further coordination. See
hughsie/libxmlb#149.
gnomesysadmins pushed a commit to GNOME/gnome-software that referenced this issue Sep 8, 2023
Providing a GError to libxmlb results in the XbMachine internals
propagating errors to the caller beyond the basic G_IO_ERROR_NOT_FOUND.

This is not ideal because it means that a lot of CPU time is spent on
creating and freeing GErrors in tight loops that is otherwise
non-essential for control flow.

Another option would be to add API to libxmlb to denote the difference
between a result set, empty set (not found), and error but that will
require further coordination. See
hughsie/libxmlb#149.
@hughsie
Copy link
Owner

hughsie commented Sep 8, 2023

FWIW, I think Christians patch is probably the right thing to do; it;s highly unlikely the images = xb_node_query (screenshot, "image", 0, NULL); call is invalid XPath... :) -- is it possible to statically allocate a GError for "not found" and then unref it? g_error_unref would have been wonderful...

@pwithnall
Copy link
Collaborator Author

it;s highly unlikely the images = xb_node_query (screenshot, "image", 0, NULL); call is invalid XPath... :)

Sure, but we do have much more complex queries than that in gnome-software, and some of them have been broken before.

is it possible to statically allocate a GError for "not found" and then unref it? g_error_unref would have been wonderful...

Nope, I’ve tried hacks like that a few times over the years and it never works out well.

@hughsie
Copy link
Owner

hughsie commented Sep 8, 2023

So the idea would be to use a in-xmlb ref'd GPtrArray to return "no results, but a valid query"? If so, that makes sense to me, but I think we'd need to make it opt-in somehow. Is there anything in XPath we can [ab]use?

@pwithnall
Copy link
Collaborator Author

pwithnall commented Sep 8, 2023

Did you see this suggestion in my original post? 👇

This might be possible without any new API: the documentation for the xb_silo_query() functions already says they return NULL if unfound, so callers should already be handling that.

Is that not correct? If it is correct, callers should already be expecting to handle NULL for ‘empty resultset’, so there shouldn’t need to be an opt-in for the changes.

@hughsie
Copy link
Owner

hughsie commented Sep 8, 2023

Right, but if we start returning NULL without setting GError lots of things are going to start exploding...

@pwithnall
Copy link
Collaborator Author

pwithnall commented Sep 8, 2023

But the documentation for xb_silo_query() already says Returns: …, or %NULL if unfound. Callers should already be expecting NULL.

I’m reading that documentation as saying it can return NULL without setting a GError. Normally the NULL return value isn’t specified for error conditions.

Are you saying it should be read the other way — that NULL can only be returned if an error is set? In that case, we could go with your static-and-empty GPtrArray suggestion. That wouldn’t have to be opt-in unless you think code is currently expecting there to always be at least one result in the returned GPtrArray if it’s non-NULL.

@hughsie
Copy link
Owner

hughsie commented Sep 8, 2023

Are you saying it should be read the other way — that NULL can only be returned if an error is set

Yup -- exactly that.

That wouldn’t have to be opt-in

I've got a nagging feeling that fwupd relies on ==NULL for no-results.

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

No branches or pull requests

2 participants