>From Ian Maxon <[email protected]>: Attention is currently required from: Ali Alsuliman. Ian Maxon has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864 )
Change subject: [ASTERIX-3283][RT] Profile micro-ops and subplans ...................................................................... Patch Set 6: (14 comments) File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/common/ClosedRecordConstructorEvalFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/5c281c86_56bb3933 PS5, Line 43: @Override : public String toString() { : return "ClosedRecordConstructor"; : } > What do we need this for? this is just to improve the appearance of the output of the profile, as it uses tostring for every factory in a micro op pipeline. File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitorJson.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/9cf9de10_54acfc97 PS5, Line 179: values().hashCode(); > Why not use Objects. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/c1acf59a_497d5294 PS5, Line 184: ((ExtendedActivityId) o).values().equals(values()); > Why not use Objects. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/7b4624b6_15b8f166 PS5, Line 207: ProfileInfo > We should rename this to OperatorProfile for clarity. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/b8f50dd0_36d6dbaf PS5, Line 214: visit > We should rename this to "updateActivity()", and rename "id" to "activityId" i renamed it to operatorId since this is supposed to be the aggregation of all instances of an operator across every partition. File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/jobgen/impl/JobBuilder.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/69ab1ca9_a85d016d PS5, Line 152: Integer k = algebraicOpBelongingToMetaAsterixOp.get(op); > You could get this "k" integer value from getLogical2PhysicalMap(). It's the > same as "v". […] Done File hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/base/ProfiledPushRuntime.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/30e8c040_c4cff89f PS5, Line 50: //don't need to add the time for each input here, because unlike other situations, : //the profiled push runtime is only used for micro-ops which are unary in/out > We actually do have non-unary micro ops. Micro union all is one example. […] addressed by adding logic to profile subplans, and micro union all File hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/meta/AlgebricksMetaOperatorDescriptor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/ac2abe39_be3bd23d PS5, Line 67: getRuntimeFactories() > How are we dealing with a IPushRuntimeFactory that produces more than 1 > IPushRuntime? One example is […] i changed this so that every runtime factory shares 1 stat. this is fine because micro op pipelines are not concurrent. this then makes it possible to aggregate stats across each push runtime of the factory into the leftmost op, which is how it appears on the plan. we don't present an instance of each framewriter, we just show the leftmost side with all of the other pipelines leading into it. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/271c25be_67058b1e PS5, Line 146: stats > What's the purpose of this "stats" being here? I only see it used down below > to get stats name. yes, that is what it's used for https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/a046aea6_e886df65 PS5, Line 227: return microOpStats[0]; > I'm not clear about this. […] because this will be the "head" of the microOp pipeline, but this shouldn't be necessary anymore with MicroOps being cosmetic and the MetaOp still being wrapped to simplify integration with the normal ops File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/ISelfProfilingOperator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/ac032d34_5352296d PS5, Line 24: ISelfProfilingOperator > Since this is for a node pushable, we should rename it to > ISelfProfilingNodePushable for clarity sin […] Done File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/IStatsContainingOperator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/045edb96_374defff PS5, Line 23: IStatsContainingOperator > Since this is for a node pushable, we should rename it to > IStatsContainingNodePushable for clarify s […] Done File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/ProfiledFrameWriter.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/f2a68876_e5b6cbcb PS5, Line 37: IFrameWriter > We can remove the "IFrameWriter" Done File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/job/profiling/StatsCollector.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/c782ece4_a3e2b87d PS5, Line 36: new ConcurrentHashMap<>(); > What happened that we have to change this to concurrent? reverted, this was due to a field being put in the outer class in AlgebricksMetaOperatorDescriptor when it belonged inside the nested class since it was modified per partition -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: trinity Gerrit-Change-Id: Ib266878bf05782506045abfadaa83b41f0f9598b Gerrit-Change-Number: 17864 Gerrit-PatchSet: 6 Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Comment-Date: Fri, 10 Nov 2023 04:15:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Ali Alsuliman <[email protected]> Gerrit-MessageType: comment
