Skip to content

Conversation

@eboasson
Copy link
Contributor

  • dds_read_instance had an unspeakably bad implementation (I plead innocent despite git blame showing my name):
    • dds_read_impl did an entirely superfluous check to see whether the instance handle was known globally, and as there is no efficient global instance handle lookup that meant a scan of all known instance handles
    • the RHC implementation is built around a hashing instance handles, but instead of using a hash lookup it scanned all instances
      And so what should have been a pretty efficient lookup ended up being two linear scans of key values ... This PR fixes that by dropping the superfluous first scan and replacing the second by a hash table lookup.
  • The code for reading/taking samples is somewhat complicated. I've refactored it a bit and in the process also removed the duplicate implementation in dds_takecdr. That interface is not quite a stable, backwards-compatible interface, but it has proved very useful on multiple occasions and I believe deserves to stay. But then the absence of a dds_readcdr companion is starting to grate. With the refactoring, adding the latter is a trivial thing. Once again: it's an evolving interface, so think before you decide to rely on it.
  • A layer of wrapper functions that did nothing more than casting the RHC pointer from the generic dds_rhc to the specific dds_rhc_default got removed as well. Those were just noise.
  • The trace optionally contains the sample contents in a readable form, but the IDL preprocessor erased so much type information that it became impossible to tell whether, e.g., a 4 byte object was originally an enumerated type, a 32-bit signed integer, a 32-bit unsigned integer, or a 32-bit floating-point number. The enums coming out as numbers is no big deal, but signed integers, and especially floating-point numbers, being printed as unsigned decimal numbers is quite annoying. So despite my reluctance to touch IDLC, this changes its output to set flags for signed and floating-point numbers. These flags only affect the printing. No requirement to regenerate anything.

* Add a flag to indicate signed integral values and one to indicate
  floating-point values
* Set these flags in the output of idlc
* Use them when printing sample contents to the trace

By encoding the information as flags in reserved bits the actual
serialization and deserialization is unaffected.

Signed-off-by: Erik Boasson <[email protected]>
Copy link
Contributor

@dpotman dpotman left a comment

Choose a reason for hiding this comment

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

LGTM, this really improves the readability of dds_rhc_default.c! I've checked the logic that was moved into the take_w_qminv and take_w_qminv_inst functions (and their read equivalent) and didn't notice anything unexpected.

}
TRACE ("take: returning %"PRIu32"\n", n);
assert (rhc_check_counts_locked (rhc, true, false));
ddsrt_mutex_unlock (&rhc->lock);
Copy link
Contributor

Choose a reason for hiding this comment

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

One would expect that this unlock is using the same conditon (lock) as used when taking this lock in the beginning of the function, but for some reason (related to the lock_samples function used to get the sample count) that's not the case. Although this PR does not change this (not so intuitive) behaviour, it would be good to add a comment here that explains the reasoning behind this. Same applies to the unlock in read_w_qinv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks!

Scanning all instances was never good for anything: the RHC is organised
as hash table on instance id (which is an alias for "instance handle")
and it was always designed to do this with a fast lookup.

Signed-off-by: Erik Boasson <[email protected]>
@eboasson
Copy link
Contributor Author

Subsumed in #512 (which targets it at security).

@eboasson eboasson closed this May 13, 2020
@eboasson eboasson deleted the read-instance-and-readcdr branch May 10, 2021 12:02
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

Successfully merging this pull request may close these issues.

2 participants