search

ome / omero-web

Django-based OMERO.web client
https://www.openmicroscopy.org/omero
17 stars 31 forks source link

Optimize obj_id_bitmask by using its own query #554

Closed chris-allan closed 4 months ago

chris-allan commented 6 months ago

Having the column oriented responses from OMERO.tables transposed into rows is quite the performance hit for the bitmask endpoint. As the endpoint also works on one column the batching and various other semantics of perform_table_query() do not apply and add needless overhead.

Consequently, the implementation has been rewritten to use its own query and the bit packing has been further optimized based on new input.

/cc @erindiel, @sbesson, @kkoz, @knabar

chris-allan commented 5 months ago

To me the undocumented previous function was internal API. Happy to keep it around and mark it deprecated if you think that's justified, @will-moore.

will-moore commented 5 months ago

No, I think it's fine. Just flagging it in case anyone else disagreed. Thx

will-moore commented 5 months ago

I'm afraid we have 2 failing tests again - see https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/99/testReport/OmeroWeb.test.integration.test_table/TestOmeroTables/test_table_bitmask_query_result0_/history/

chris-allan commented 4 months ago

51d8ba745b5f5ec248f29c258a7d14150017e5ce should fix the integration test failures; the previous implementation was kind of weird and the behaviour we're now mimicing was undocumented but it's been like that for several years and the integration tests depend on it. We can decide in the future if we want to make more drastic changes and properly fail if a column is unknown.

79c23056136bc929e4ebd57d24600780c92921c5 is a little more sinister as the previous implementation would have had weird behaviour around negative values. We aren't specifically testing for this sort of strange data layout but the implementation now mimics what was there previously and again we can decide in the future if we want to explicitly disallow negative indexes.

knabar commented 4 months ago

@chris-allan Thank you for tracking down these issues.

Once the new API endpoints are available, would we consider deprecating the bitmask call altogether?

chris-allan commented 4 months ago

Once the new API endpoints are available, would we consider deprecating the bitmask call altogether?

Potentially, yes. Once they're in place I would anticipate very similar performance characteristics to bitmask with much more appropriate functionality.

chris-allan commented 4 months ago

Test cases now passing: