>From Ali Alsuliman <[email protected]>: Attention is currently required from: Ian Maxon. Ali Alsuliman has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864 )
Change subject: [ASTERIX-3283] Profile micro-ops ...................................................................... Patch Set 5: (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/dafc17e8_142e29a6 PS5, Line 43: @Override : public String toString() { : return "ClosedRecordConstructor"; : } What do we need this for? 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/23c9738a_9586c2b5 PS5, Line 179: values().hashCode(); Why not use Objects.hash()? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/587abe82_b96c87cf PS5, Line 184: ((ExtendedActivityId) o).values().equals(values()); Why not use Objects.equals()? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/2e92c629_db3c918f PS5, Line 207: ProfileInfo We should rename this to OperatorProfile for clarity. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/ca42b773_62fa0b8b PS5, Line 214: visit We should rename this to "updateActivity()", and rename "id" to "activityId" 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/92a130d4_4623c6f3 PS5, Line 152: Integer k = algebraicOpBelongingToMetaAsterixOp.get(op); You could get this "k" integer value from getLogical2PhysicalMap(). It's the same as "v". Pass it from getLogical2PhysicalMap() along with "op" and name it metaOpIdx. 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/e4701532_127e13f7 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. You can see an example of a query here: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/2277/ Run the example query and check how your current code constructs the profiling push times. 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/db8870d1_4f4a5b74 PS5, Line 67: getRuntimeFactories() How are we dealing with a IPushRuntimeFactory that produces more than 1 IPushRuntime? One example is the micro union all (MicroUnionAllRuntimeFactory). https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/249c1a18_ac37b44d PS5, Line 146: stats What's the purpose of this "stats" being here? I only see it used down below to get stats name. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17864/comment/75d13154_d1cccba5 PS5, Line 227: return microOpStats[0]; I'm not clear about this. Why 0? 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/39edf82a_042cdcab PS5, Line 24: ISelfProfilingOperator Since this is for a node pushable, we should rename it to ISelfProfilingNodePushable for clarity since an operator can have multiple node pushables (multiple activities). 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/31309a5e_447d00c9 PS5, Line 23: IStatsContainingOperator Since this is for a node pushable, we should rename it to IStatsContainingNodePushable for clarify since an operator can have multiple node pushables (multiple activities). 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/bfa22b58_7c9899aa PS5, Line 37: IFrameWriter We can remove the "IFrameWriter" 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/b6b3b466_3385f3bd PS5, Line 36: new ConcurrentHashMap<>(); What happened that we have to change this to concurrent? -- 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: 5 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: Ian Maxon <[email protected]> Gerrit-Comment-Date: Sat, 21 Oct 2023 05:08:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
