On Tue, May 13, 2014 at 10:42 PM, Anthony Scopatz
<notifications@github.com>wrote:

> In src/hdf5_back.cc:
>
> > +
> > +template <>
> > +Digest Hdf5Back::VLWrite<Blob, BLOB>(const Blob& x) {
> > +  hasher_.Clear();
> > +  hasher_.Update(x);
> > +  Digest key = hasher_.digest();
> > +  hid_t keysds = VLDataset(BLOB, true);
> > +  hid_t valsds = VLDataset(BLOB, false);
> > +  if (vlkeys_[BLOB].count(key) == 1)
> > +    return key;
> > +  AppendVLKey(keysds, BLOB, key);
> > +  InsertVLVal(valsds, BLOB, key, x.str());
> > +  return key;
> > +}
> > +
> > +hid_t Hdf5Back::VLDataset(DbTypes dbtype, bool forkeys) {
>
> In the other comment @gidden <https://github.com/gidden>, you mentioned
> breaking the long functions with switch-case statements into many
> functions. I believe that this is bad idea (which is why I didn't write
> them this way to begin with ^_^).
>
To be clear, separating the long switch/case block into its own function.

>  From what I could gather, the backends are a performance critical part
> of the code base.
>
We have done next-to-no profiling. I'm not convinced this is the case.
Could you cite why you gathered it is?

> These switch-cases (eps in Query, CreateTable, etc) are executed for each
> column and each row. Function call overhead is roughly ~100 cycles the last
> time I checked. Moving to private member functions would more than negate
> the benefits of having the DbTypes abstraction.
>
I am not familiar enough with this level of optimization to comment further.

> These switch-cases are going to be big if they are going to be efficient -
> and I believe that they should be. That said, to improve readability I
> wouldn't be opposed to moving these functions to their own *.cc files.
> Alternatively, I could move just the switch-cases to their own files and
> then #include them. I also wouldn't be opposed to dropping some gotos
> (though #include is faster).
>
> I fully expect some of these functions to be 1000+ lines if and when they
> are fully implemented for all types. I had considered moving them to their
> own implementation files but wanted to gain consensus that this was an OK
> thing to do before proceeding.
>
I'll let @rwcarlsen comment.

> —
> Reply to this email directly or view it on GitHub<https://github.com/cyclus/cyclus/pull/840/files#r12619767>
> .
>



-- 
Matthew Gidden
Ph.D. Candidate, Nuclear Engineering
The University of Wisconsin -- Madison
Ph. 225.892.3192