Opened 7 years ago

Closed 7 years ago

#2565 closed enhancement (fixed)

Refactor RR param parsing code in MasterLoader

Reported by: muks Owned by: muks
Priority: medium Milestone: Sprint-20130108
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 1
Total Hours: 0 Internal?: no

Description

This is a continuation of #2431. It involves refactoring of code inside MasterLoader::MasterLoaderImpl::loadIncremental().

This ticket will start from a review of existing code. Setting it to current sprint as the code is ready and we don't want to postpone its review and merge.

Subtickets

Change History (16)

comment:1 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei
  • Status changed from new to reviewing

trac2565 branch is up for review.

comment:2 in reply to: ↑ 1 Changed 7 years ago by jinmei

Replying to muks:

trac2565 branch is up for review.

As we discussed in #2429, I suggest using boost::optional. At least
it doesn't make sense to use different approaches for RRTTL and for
RRClass for essentially the same purpose.

comment:3 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:4 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

I have prepared the branch using boost::optional as you have recommended, but I think it is not necessary in this case.

  • The bare version (returning NULL instead of MaybeRRClass()) that we talked about on Jabber seems to be enough for this particular case. The current code in the branch (with MaybeRRClass) has the same effect in our use-case.
  • Even in the RRTTL case, using MaybeRRTTL may not need additional allocation but does the caller not do an object copy which seems to defeat this optimization? Is introducing boost::optional required?
  • Our use of boost::optional does not seem to match the purpose it was written for (i.e., to represent a return value that does not lie in the range of the return type). In this case, we don't have this problem as NULL serves this purpose.

comment:5 in reply to: ↑ 4 Changed 7 years ago by jinmei

Replying to muks:

I have prepared the branch using boost::optional as you have recommended, but I think it is not necessary in this case.

  • The bare version (returning NULL instead of MaybeRRClass()) that we talked about on Jabber seems to be enough for this particular case.

How can we avoid allocating memory every time then?

  • Even in the RRTTL case, using MaybeRRTTL may not need additional allocation but does the caller not do an object copy which seems to defeat this optimization? Is introducing boost::optional required?

I don't understand this. Is this about the overhead of copying an
RRTTL object? If so, I believe it should be very cheap because it
only contains a trivial uint32_t member and we only need to copy it in
a trivial way.

  • Our use of boost::optional does not seem to match the purpose it was written for (i.e., to represent a return value that does not lie in the range of the return type). In this case, we don't have this problem as NULL serves this purpose.

I thought we already covered this topic, so I'm not sure why we are
repeating... Yes, NULL suffices, but that would require either
memory allocation or breaking class integrity. I wanted avoid these,
and boost::optional was a tool for that.

Is there anything still not clear enough?

comment:6 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:7 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

I follow now what you mean about allocation (on the heap). I agree that the single uint32_t makes the copy cheap in this case. I want to check the performance overhead of boost::optional vs. the new and if it matters in the context of the current code, but when I have time to spend on it.

Please review the code in the branch as it uses boost::optional like in the RRTTL code.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by jinmei

Replying to muks:

I follow now what you mean about allocation (on the heap). I agree that the single uint32_t makes the copy cheap in this case. I want to check the performance overhead of boost::optional vs. the new and if it matters in the context of the current code, but when I have time to spend on it.

Please review the code in the branch as it uses boost::optional like in the RRTTL code.

Okay, this is the review result:

changelog

Do we need an entry?

rrclass-placeholder.h

  • s/details/detailed/?
        /// this reason this function intentionally omits the capability of
        /// delivering details reason for the parse failure, such as in the
    
    (If so, this was probably an error in the RRTTL text which seemed to be mostly copy-pasted here. So both should be fixed)
  • on a related note, the documentation for RRTTL and RRClass is essentially the same, just parameterized with the class name. Ideally they should be unified according to DRY. But I don't have a good idea for that right now, so if you don't have one either, maybe we should live with it for now.

rrparamregistry

This is actually beyond the scope of this task, but I think it's
better to keep the signature/interface of textToXXXCode look
consistent for XXX being Type and Code. So I'd extend textToTypeCode
in the same way.

I'd also note (in the doc) the rationale of the interface, especially
about why we simply don't throw and return boolean when it fails to
convert the text.

master_loader.cc

  • This seems to be a bit more expensive than ideal:
                if (rrclass != zone_class_) {
    
    If I understand it correctly, this will construct another optional object from zone_class_ (using an implicit constructor) and calls operator!=. I suspect it's slightly better in performance:
                if (*rrclass != zone_class_) {
    
    (and, in general, I'd prefer explicit representations even if its performance advantage is marginal)
  • I guess we should simply throw InternalException (and remove the comment here), just like the previous version:
                    // It doesn't really matter much what type of exception
                    // we throw, we catch it just below.
                    isc_throw(isc::BadValue, "Class mismatch: " << rrclass <<
                              "vs. " << zone_class_);
    

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:10 in reply to: ↑ 8 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

changelog

Do we need an entry?

I don't think we need a ChangeLog entry. Even though there are some libdns++ changes, these don't seem to modify existing public API.

rrclass-placeholder.h

  • s/details/detailed/?
        /// this reason this function intentionally omits the capability of
        /// delivering details reason for the parse failure, such as in the
    
    (If so, this was probably an error in the RRTTL text which seemed to be mostly copy-pasted here. So both should be fixed)

Both cases are fixed now.

  • on a related note, the documentation for RRTTL and RRClass is essentially the same, just parameterized with the class name. Ideally they should be unified according to DRY. But I don't have a good idea for that right now, so if you don't have one either, maybe we should live with it for now.

I don't have any ideas on how to unify documentation as stated too.

rrparamregistry

This is actually beyond the scope of this task, but I think it's
better to keep the signature/interface of textToXXXCode look
consistent for XXX being Type and Code. So I'd extend textToTypeCode
in the same way.

Done.

I'd also note (in the doc) the rationale of the interface, especially
about why we simply don't throw and return boolean when it fails to
convert the text.

Done.

master_loader.cc

  • This seems to be a bit more expensive than ideal:
                if (rrclass != zone_class_) {
    
    If I understand it correctly, this will construct another optional object from zone_class_ (using an implicit constructor) and calls operator!=. I suspect it's slightly better in performance:
                if (*rrclass != zone_class_) {
    
    (and, in general, I'd prefer explicit representations even if its performance advantage is marginal)

Done.

  • I guess we should simply throw InternalException (and remove the comment here), just like the previous version:
                    // It doesn't really matter much what type of exception
                    // we throw, we catch it just below.
                    isc_throw(isc::BadValue, "Class mismatch: " << rrclass <<
                              "vs. " << zone_class_);
    

Done.

I've also pushed some other misc. updates.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by jinmei

Replying to muks:

changelog

Do we need an entry?

I don't think we need a ChangeLog entry. Even though there are some libdns++ changes, these don't seem to modify existing public API.

Adding a new public interface could also be worth a changelog entry,
but in this case we probably don't need it.

Other minor comments on the revised branch:

  • I'd change "We return" to "It returns" for consistency:
        /// We return \c false and avoid simply throwing an exception in the
        /// case of an error so that the code is performant in some
        /// situations.
    
    I didn't understand this: "the code is performant in some situations".
  • In this context we know it's RR type, so I'd say "Unrecognized RR type string":
        if (!RRParamRegistry::getRegistry().textToTypeCode(type_str, typecode_)) {
            isc_throw(InvalidRRType,
                      "Unrecognized RR parameter string: " + type_str);
        }
    
    For that matter, same point applies to the RRClass constructor.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by muks

Hi Jinmei

Replying to jinmei:

  • I'd change "We return" to "It returns" for consistency:
        /// We return \c false and avoid simply throwing an exception in the
        /// case of an error so that the code is performant in some
        /// situations.
    
    I didn't understand this: "the code is performant in some situations".

I've updated it with this:

-    /// We return \c false and avoid simply throwing an exception in the
-    /// case of an error so that the code is performant in some
-    /// situations.
+    /// It returns \c false and avoids throwing an exception in the case
+    /// of an error to avoid the exception overhead in some situations.
  • In this context we know it's RR type, so I'd say "Unrecognized RR type string":
        if (!RRParamRegistry::getRegistry().textToTypeCode(type_str, typecode_)) {
            isc_throw(InvalidRRType,
                      "Unrecognized RR parameter string: " + type_str);
        }
    
    For that matter, same point applies to the RRClass constructor.

Both are updated now.

comment:13 in reply to: ↑ 12 Changed 7 years ago by jinmei

Looks good, please merge.

comment:14 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:15 Changed 7 years ago by jinmei

  • Add Hours to Ticket changed from 0 to 1

comment:16 Changed 7 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master branch in commit bef01123fb5dfaee927bc58da465109240811a18:

* 025db8d [2565] Update comment about exception overhead
* 00845ee [2565] Specify parameter kind in exception messages
* 080d47d [2565] Add rationale for why bool is returned instead of throwing an exception
* 9f5102a [2565] Rename constructor arguments to look nicer
* 0758ead [2565] Update textToTypeCode() to match textToClassCode()
* 9ab7213 [2565] Rename function argument
* ffe973e [2565] Fix indenting and untabify
* 857d7fb [2565] Fix exception message returned when classes don't match
* 514b87c [2565] Throw InternalException instead of isc::BadValue
* 6d28690 [2565] Use the wrapped RRClass directly, in the comparison
* 029be67 [2565] Fix grammar in methods' documentation
* 5ec876c [2565] style cleanups: constify; combine short lines
* 09dc37b [2565] constify
* b0bc2d3 [2565] Use boost::optional in the newly introduced RRClass factory
* 1b69710 [2565] Add proto for RRClass::fromText()
* 4d6495a [2565] Avoid redundant construction in parseRRParams()
* 530e62b [2565] Add RRClass.fromText()
* ed7622d [2565] Refactor code so that RRParamRegistry::textToClassCode() does not throw

Resolving as fixed. Thank you for the reviews Jinmei.

Note: See TracTickets for help on using tickets.