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

C flatcc builder (put) optimization #8

Open
vaind opened this issue Jun 23, 2020 · 6 comments
Open

C flatcc builder (put) optimization #8

vaind opened this issue Jun 23, 2020 · 6 comments
Labels
enhancement New feature or request

Comments

@vaind
Copy link
Contributor

vaind commented Jun 23, 2020

The generated code currently calls flatcc_builder_finalize_aligned_buffer() which returns a copy of the builder buffer. The returned buffer must be freed by calling flatcc_builder_aligned_free(), in addition to freeing the builder using flatcc_builder_clear(). Have a look at the following sample from README.md

obx_id task_put(OBX_box* box, Task* task) {
    flatcc_builder_t builder;
    flatcc_builder_init(&builder);

    size_t size = 0;
    void* buffer = NULL;

    // Note: Task_to_flatbuffer() is provided by the generated code
    obx_id id = 0;
    if (Task_to_flatbuffer(&builder, task, &buffer, &size)) {
        id = obx_box_put_object(box, buffer, size, OBXPutMode_PUT);  // returns 0 on error
    }

    flatcc_builder_clear(&builder);
    if (buffer) flatcc_builder_aligned_free(buffer);

    if (id == 0) {
        // TODO: won't be able to print the right error if it occurred in Task_to_flatbuffer(), 
        //  i.e. outside objectbox
        print_last_error();
    } else {
        task->id = id;  // Note: we're updating the ID on new objects for convenience
    }

    return id;
}

We should have a look if/how we can optimize this, in terms of performance (avoiding the memory copy if possible) and usability.

@vaind vaind added the enhancement New feature or request label Jun 23, 2020
@vaind
Copy link
Contributor Author

vaind commented Jun 23, 2020

Note: I'm not sure the copy could be avoided, considering how flatcc_builder works - it doesn't produce the final output on the first call, only when calling "finalize". Maybe there's another mode similar to other flatbuffers implementations producing the final buffer right away without the in-between "emitter"?

Maybe @mikkelfj has some suggestions?

@mikkelfj
Copy link

mikkelfj commented Jun 23, 2020

The copy can be avoided, to a degree:

You can call a different version of finalize that accepts a fixed size buffer, and you can call yet another function to detect the size of the buffer required.

However, the default backend emitter of the builder is collecting data in chained pages, not in contiguous memory, so this memory must be copied out in order to read the data as a buffer. And this data generally needs to be aligned to the largest alignment unit used in the buffer. Usually 8 bytes suffice, but char mybuf[4096]; will not be aligned. While this works on most CPU's today, the verifier will still complain due to semantics.

If you want to further optimize, you can provide you own emitter backend. It is essentially a simple callback that receives an iovec array of small chunks of data to copy. There are a few emitter examples in the test directory.

@greenrobot
Copy link
Member

greenrobot commented Jun 23, 2020

Another topic: are we able to reuse flatcc_builder_t? I don't recall if we can cache it.

PS.: flatcc related optimizations can be done after 1.0

@mikkelfj
Copy link

I can't answer that, but the mentioned list of chained pages will be recycled efficiently if you do cache the builder, and so will several internal stacks. You can control how much memory the builder retains between builds in case you have a large spike. See custom version of the reset call.

@vaind
Copy link
Contributor Author

vaind commented Jul 15, 2020

Also see the discussion about an overload for generated _put() and _get() functions #11 (review)

@vaind
Copy link
Contributor Author

vaind commented Feb 15, 2021

Also to keep in mind: there's an internal discussion/feature request for providing flatbuffers serialization as part of the C-API. It's mainly for other language bindings but it would also solve this issue (data copy), simplify the code generator (we could have the same code for C++ and C) and drop the C flatc dependency for end-users altogether. Internal issue number: 363

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants