New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HPCC-17326 Rationalise the use of StTimeElapsed etc #10088
Conversation
https://track.hpccsystems.com/browse/HPCC-17326 |
Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>
@shamser please review |
Automated Smoketest: ❌
Install hpccsystems-platform-community_6.5.0-trunk0.el7.x86_64.rpm Unit tests result:
Regression test result:
Errors:
HPCC Stop: OK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should wutool->test suite need be updated with the new scope?
@@ -5960,11 +5953,6 @@ void CLocalWorkUnit::_loadStatistics() const | |||
IConstWUStatisticIterator& CLocalWorkUnit::getStatistics(const IStatisticsFilter * filter) const | |||
{ | |||
CriticalBlock block(crit); | |||
//This should be deleted in version 6.0 when support for 4.x is no longer required | |||
legacyTimings.loadBranch(p,"Timings"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghalliday Does the removal of support for legacy timings need to be documented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so - it would be very unusual to have a workunit from 4.0 in a 7.0 system. If they did the only problem would be timings didn't appear any more. @richardkchapman do we document things like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
@@ -2525,8 +2531,6 @@ void EclCC::processBatchedFile(IFile & file, bool multiThreaded) | |||
if (info.wu && | |||
(info.wu->getDebugValueBool("generatePartialOutputOnError", false) || info.queryErrorProcessor().errCount() == 0)) | |||
{ | |||
WuStatisticTarget statsTarget(info.wu, "eclcc"); | |||
gatherTransformStats(statsTarget); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghalliday Another change in behaviour.. require documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has now been made conditional rather than unconditional, and the option has been added to the eclcc help.
@shamser what kind of documentation do you suggest?
} | ||
|
||
gatherExplicitMatched(expr); | ||
doBuildParseTransform(instance->startctx, expr); // also gathers all the MATCHED() definitions. | ||
doBuildParseSearchText(instance->startctx, expr); | ||
doBuildParseValidators(instance->nestedctx, expr); | ||
doBuildParseExtra(instance->startctx, expr); | ||
noteFinishedTiming("compile:generate PARSE:prepare", startPrepareCycles); | ||
if (options.timeTransforms) | ||
noteFinishedTiming("compile:PARSE:prepare", startPrepareCycles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is 'PARSE' capitalized? Seems inconsistant with other timers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it corresponds to the PARSE keyword.
I could change to lower case, but then it might be confused with the eclcc parser, so I would keep it as is, even though it isn't consistent.
@@ -727,7 +728,8 @@ ABoundActivity * HqlCppTranslator::doBuildActivityParse(BuildCtx & ctx, IHqlExpr | |||
WARNING1(CategoryEfficiency, HQLWRN_GrammarIsAmbiguous, instance->activityId); | |||
|
|||
doBuildParseCompiled(instance->classctx, buffer); | |||
noteFinishedTiming("compile:generate PARSE:compile", startCompileCycles); | |||
if (options.timeTransforms) | |||
noteFinishedTiming("compile:PARSE:compile", startCompileCycles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, not sure why 'PARSE' is capitalized?
@@ -760,7 +762,8 @@ ABoundActivity * HqlCppTranslator::doBuildActivityParse(BuildCtx & ctx, IHqlExpr | |||
nlpParse = NULL; | |||
buildInstanceSuffix(instance); | |||
buildConnectInputOutput(ctx, instance, boundDataset, 0, 0); | |||
noteFinishedTiming("compile:generate PARSE", startCycles); | |||
if (options.timeTransforms) | |||
noteFinishedTiming("compile:PARSE", startCycles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, not sure why 'PARSE' is capitalized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghalliday Reviewed... a few comments added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardkchapman Code reviewed.. Looks good
@shamser see replies. |
I don't think there is any need to change compile:parseTime in wutool |
@ghalliday @richardkchapman Code reviewed.. Looks good |
Signed-off-by: Gavin Halliday gavin.halliday@lexisnexis.com
Type of change:
Checklist:
Testing:
Run compiler regression suites, and submitted queries to eclccserver and eclserver. (eclccserver has problems with the types of some of the timings, but that will be addressed in HPCC-17693).