Which #ifdef are you referring to? -----Original Message----- From: HTCondor-devel [mailto:[email protected]] On Behalf Of Brian Bockelman Sent: Monday, March 13, 2017 10:14 AM To: Greg Thain <[email protected]> Cc: [email protected] Subject: Re: [HTCondor-devel] Need for dynamic slots in top-level collector?
> On Mar 13, 2017, at 9:31 AM, Brian Bockelman <[email protected]> wrote: > > Hi Greg, > > I would be OK if the collector never handled a query inline - either doing an > EAGAIN or just queuing them up internally for some finite amount of time. > > In this case, the query was inlined because the child processes pushed the > collector into swap. Once swap was involved and memory operations took > seconds, well, the whole thing kind of went to hell until the master shot the > involved processes. > > Going in a different direction, I tried a "poor man's profiling" (pstack > repeatedly) and found lots of places to potentially squeeze out performance. > > - ConvertDefaultIPToSocketIP is called twice for every attribute in the ad. > This does a bunch of string manipulation, but _also_ does at least one > param_boolean call per invocation. Nothing like doing this a few million > times per query: > > #0 0x00007f85963e0f18 in next_config_macro () from > /usr/lib64/libcondor_utils_8_7_0.so > #1 0x00007f8596418803 in expand_macro(char const*, macro_set&, > macro_eval_context&) () from /usr/lib64/libcondor_utils_8_7_0.so > #2 0x00007f85964e3317 in param_ctx () from > /usr/lib64/libcondor_utils_8_7_0.so > #3 0x00007f85964e4749 in param () from /usr/lib64/libcondor_utils_8_7_0.so > #4 0x00007f85964e4a8e in param_boolean(char const*, bool, bool, > compat_classad::ClassAd*, compat_classad::ClassAd*, bool) () from > /usr/lib64/libcondor_utils_8_7_0.so > #5 0x00007f85963ea346 in ConvertDefaultIPToSocketIP(char const*, > std::basic_string<char, std::char_traits<char>, std::allocator<char> >&, > Stream&) () from /usr/lib64/libcondor_utils_8_7_0.so > > - GetInternalReferences looks surprisingly expensive. Since the negotiator > typically whitelists attributes for claimed ads, is there room for > improvement? > > - While it obviously showed up, I was surprised by how small of a percentage > of the time was spent in classad::ClassAdUnParser::Unparse. Maybe 30%? 50%? > > - prepare_crypto_for_secret_is_noop() involves a modest amount of string > parsing, is called once per attribute, yet is a property of the connection > itself. It only needs to be called once per invocation of putClassAd - > perhaps can be cached too? > > - The std::string buffered output is released / allocated between each > attribute. It might be reasonable to recycle the allocation on the heap and > reserve a modest amount of memory (1KB?)? > > - There's a bit of slop in the way we evaluate string values. Consider the > following traceback: > > #1 0x00007f8595ea6ca9 in classad::Value::CopyFrom(classad::Value const&) () > from /usr/lib64/libclassad.so.9 > #2 0x00007f8595e93580 in classad::Literal::_Evaluate(classad::EvalState&, > classad::Value&) const () from /usr/lib64/libclassad.so.9 > #3 0x00007f8595e82501 in classad::ExprTree::Evaluate(classad::EvalState&, > classad::Value&) const () from /usr/lib64/libclassad.so.9 > #4 0x00007f8595e66ea0 in > classad::ClassAd::EvaluateAttr(std::basic_string<char, > std::char_traits<char>, std::allocator<char> > const&, classad::Value&) const > () from /usr/lib64/libclassad.so.9 > #5 0x00007f8595e66fe9 in > classad::ClassAd::EvaluateAttrString(std::basic_string<char, > std::char_traits<char>, std::allocator<char> > const&, > std::basic_string<char, std::char_traits<char>, std::allocator<char> >&) > const () from /usr/lib64/libclassad.so.9 > > I think we copy the string twice time: once from the literal to the value, > then from the value to the user-provided string (which is reallocated for > each attribute). It would be reasonable to short-circuit this (check to see > if the attribute's expression / cached expression is a literal string type, > then make a copy directly to the user buffer); it might be possible to avoid > it in all cases. Given that we have const-ness all around, we could take a > step further and introduce a EvaluateLiteralString that does zero-copy for > strings. > There's another twist on this problem in Unparse: #0 0x00007f859513d258 in std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) () from /usr/lib64/libstdc++.so.6 #1 0x00007f8595ea6ca9 in classad::Value::CopyFrom(classad::Value const&) () from /usr/lib64/libclassad.so.9 #2 0x00007f8595e9d647 in classad::ClassAdUnParser::Unparse(std::basic_string<char, std::char_traits<char>, std::allocator<char> >&, classad::ExprTree const*) () from /usr/lib64/libclassad.so.9 #3 0x00007f85964da5d6 in _putClassAd(Stream*, classad::ClassAd&, int) () from /usr/lib64/libcondor_utils_8_7_0.so #4 0x00007f85964daa78 in putClassAd(Stream*, classad::ClassAd&, int, std::set<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, classad::CaseIgnLTStr, std::allocator<std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const*) () from /usr/lib64/libcondor_utils_8_7_0.so We make a copy of the classad::Value (memory allocation, copy the string) in `Unparse`, then make a copy of the string (memory allocation, copy the string), then copy the string to the user-provided buffer (memory allocation, copy the string). Oh - and finally, we make a copy in putClassAd to the CEDAR socket buffer. Seems TJ already discovered this when pulling the Factor out of the value, but left the fix #ifdef'd out? Brian _______________________________________________ HTCondor-devel mailing list [email protected] https://lists.cs.wisc.edu/mailman/listinfo/htcondor-devel _______________________________________________ HTCondor-devel mailing list [email protected] https://lists.cs.wisc.edu/mailman/listinfo/htcondor-devel
