Baunsgaard commented on code in PR #2435:
URL: https://github.com/apache/systemds/pull/2435#discussion_r3560165972
##########
src/main/java/org/apache/sysds/hops/codegen/SpoofCompiler.java:
##########
@@ -19,11 +19,23 @@
package org.apache.sysds.hops.codegen;
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.Map.Entry;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
Review Comment:
can we avoid moving these imports around in this PR?
##########
src/main/java/org/apache/sysds/hops/DataOp.java:
##########
@@ -19,13 +19,14 @@
package org.apache.sysds.hops;
+import static org.apache.sysds.parser.DataExpression.FED_RANGES;
+
import java.util.HashMap;
Review Comment:
Keep the import reordering of this file.
It clearly needed cleanup.
##########
src/main/java/org/apache/sysds/hops/codegen/SpoofCompiler.java:
##########
@@ -538,22 +537,21 @@ public static ArrayList<Hop> optimize(ArrayList<Hop>
roots, boolean recompile)
LOG.warn("CPlan " +
tmp.getValue().getVarname() + " not supported by SPOOF CUDA");
}
- //explain debug output cplans or
generated source code
- if( LOG.isInfoEnabled() ||
DMLScript.EXPLAIN.isHopsType(recompile) ) {
- LOG.info("Codegen EXPLAIN
(generated cplan for HopID: " + cplan.getKey() +
- ", line
"+tmp.getValue().getBeginLine() + ", hash="+tmp.getValue().hashCode()+"):");
-
LOG.info(tmp.getValue().getClassname()
- +
Explain.explainCPlan(cplan.getValue().getValue()));
+ // explain debug output cplans or
generated source code
+ // (explicit guard:
Explain.explainCPlan is expensive and evaluated eagerly)
Review Comment:
remove this filling comment.
It makes sense that we do not do the check for this logging.
##########
src/main/java/org/apache/sysds/runtime/controlprogram/caching/CacheableData.java:
##########
@@ -905,15 +904,13 @@ public synchronized void exportData(String fName, String
outputFormat, FileForma
* @param formatProperties file format properties
*/
public synchronized void exportData (String fName, String outputFormat,
int replication, FileFormatProperties formatProperties) {
- if( LOG.isTraceEnabled() )
- LOG.trace("Export data "+hashCode()+" "+fName);
+ LOG.trace("Export data {} {}", hashCode(), fName);
long t0 = DMLScript.STATISTICS ? System.nanoTime() : 0;
//prevent concurrent modifications
if ( !isAvailableToRead() )
throw new DMLRuntimeException("MatrixObject not
available to read.");
- if( LOG.isTraceEnabled() )
- LOG.trace("Exporting " + this.getDebugName() + " to " +
fName + " in format " + outputFormat);
+ LOG.trace("Exporting {} to {} in format {}",
this.getDebugName(), fName, outputFormat);
Review Comment:
we need to doublecheck that this.getDebugName is not constructing a string,
otherwise, we might want to instead introduce a toString() method for
CacheableData instead. This would avoid the eager string construction of the
caller.
##########
src/main/java/org/apache/sysds/runtime/controlprogram/caching/CacheableData.java:
##########
@@ -1076,28 +1072,27 @@ protected boolean isBlobPresent() {
*/
protected void restoreBlobIntoMemory() {
String cacheFilePathAndName = getCacheFilePathAndName();
- long begin = LOG.isTraceEnabled() ? System.currentTimeMillis()
: 0;
-
- if( LOG.isTraceEnabled() )
- LOG.trace ("CACHE: Restoring matrix... " + hashCode()
+ " HDFS path: " +
- (_hdfsFileName == null ? "null"
: _hdfsFileName) + ", Restore from path: " + cacheFilePathAndName);
-
- if (_data != null)
- throw new DMLRuntimeException(cacheFilePathAndName + "
: Cannot restore on top of existing in-memory data.");
+ long begin = LOG.currentTimeMillisIfTraceEnabled();
Review Comment:
I feel like adding this currentTImeMillis is a bit overkill. but fine we can
keep it.
##########
src/main/java/org/apache/sysds/runtime/controlprogram/caching/CacheableData.java:
##########
@@ -1043,8 +1040,7 @@ else if( getRDDHandle()!=null &&
getRDDHandle().isPending()
else
{
//CASE 4: data already in hdfs (do nothing, no need for
export)
- if( LOG.isTraceEnabled() )
- LOG.trace(this.getDebugName() + ": Skip export
to hdfs since data already exists.");
+ LOG.trace("{}: Skip export to hdfs since data already
exists.", this.getDebugName());
Review Comment:
same doublecheck the getDebugName
##########
src/main/java/org/apache/sysds/runtime/controlprogram/paramserv/FederatedPSControlThread.java:
##########
@@ -147,11 +146,9 @@ public void setup(double weightingFactor) {
if(_runtimeBalancing == PSRuntimeBalancing.BASELINE)
_cycleStartAt0 = true;
- if( LOG.isInfoEnabled() ) {
- LOG.info("Setup config for worker " +
this.getWorkerName());
- LOG.info("Batch size: " + _batchSize + " possible
batches: " + _possibleBatchesPerLocalEpoch
- + " batches to run: " +
_numBatchesPerEpoch + " weighting factor: " + _weightingFactor);
- }
+ LOG.info("Setup config for worker {}", this.getWorkerName());
Review Comment:
doublecheck it does not allocate, otherwise make a case class, and have a
overloaded toString()
##########
src/main/java/org/apache/sysds/runtime/instructions/cp/ParamservBuiltinCPInstruction.java:
##########
@@ -611,12 +608,11 @@ private void partitionLocally(PSScheme scheme,
ExecutionContext ec, List<LocalPS
.doPartitioning(workers.size(),
features.acquireReadAndRelease(), labels.acquireReadAndRelease());
List<MatrixObject> pfs = result.pFeatures;
List<MatrixObject> pls = result.pLabels;
- if (pfs.size() < workers.size()) {
- if (LOG.isWarnEnabled()) {
- LOG.warn(String.format("There is only %d
batches of data but has %d workers. "
- + "Hence, reset the number of workers
with %d.", pfs.size(), workers.size(), pfs.size()));
- }
- if (getUpdateType().isSBP() && pfs.size() <=
getNumBackupWorkers()) {
+ if(pfs.size() < workers.size()) {
+ LOG.warn(
+ "There is only {} batches of data but has {}
workers. " + "Hence, reset the number of workers with {}.",
+ pfs.size(), workers.size(), pfs.size());
Review Comment:
Since some of them are reused from the if statement, it could maybe be
cleaner to make variables?
##########
src/main/java/org/apache/sysds/utils/ParameterizedLogger.java:
##########
@@ -0,0 +1,170 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sysds.utils;
+
+import java.util.IllegalFormatException;
+import java.util.Locale;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
+/**
+ * Logger adapter for commons-logging with lightweight '{}' parameter
formatting. In addition to SLF4J-style '{}'
+ * placeholders, '{%fmt}' placeholders apply a {@link String#format} spec to
the argument, e.g. {@code LOG.debug("ratio:
+ * {%10.3f}", ratio)}. Note that arguments are still evaluated eagerly at the
call site, so keep an explicit
+ * {@code isDebugEnabled()} guard around calls with expensive arguments.
+ */
+public class ParameterizedLogger {
+ private final Log _log;
+
+ public ParameterizedLogger(Log log) {
+ _log = log;
+ }
+
+ public static ParameterizedLogger getLogger(Class<?> cls) {
+ return new
ParameterizedLogger(LogFactory.getLog(cls.getName()));
+ }
+
+ public long currentTimeMillisIfTraceEnabled() {
+ return _log.isTraceEnabled() ? System.currentTimeMillis() : 0;
+ }
+
+ public boolean isTraceEnabled() {
+ return _log.isTraceEnabled();
+ }
+
+ public boolean isDebugEnabled() {
+ return _log.isDebugEnabled();
+ }
+
+ public boolean isInfoEnabled() {
+ return _log.isInfoEnabled();
+ }
+
+ public void trace(String pattern, Object... args) {
+ if(_log.isTraceEnabled())
+ _log.trace(format(pattern, args));
+ }
+
+ public void trace(Object message) {
+ _log.trace(message);
+ }
+
+ public void debug(String pattern, Object... args) {
+ if(_log.isDebugEnabled())
+ _log.debug(format(pattern, args));
+ }
+
+ public void debug(Object message) {
+ _log.debug(message);
+ }
+
+ public void debug(Object message, Throwable t) {
+ _log.debug(message, t);
+ }
+
+ public void info(String pattern, Object... args) {
+ if(_log.isInfoEnabled())
+ _log.info(format(pattern, args));
+ }
+
+ public void info(Object message) {
+ _log.info(message);
+ }
+
+ public void warn(String pattern, Object... args) {
+ if(_log.isWarnEnabled())
+ _log.warn(format(pattern, args));
+ }
+
+ public void warn(Object message) {
+ _log.warn(message);
+ }
+
+ public void error(String pattern, Object... args) {
+ if(_log.isErrorEnabled())
+ _log.error(format(pattern, args));
+ }
+
+ public void error(Object message) {
+ _log.error(message);
+ }
+
+ public void error(Object message, Throwable t) {
+ _log.error(message, t);
+ }
+
+ /**
+ * Substitutes {@code {}} and {@code {%fmt}} placeholders in {@code
pattern} with {@code args}. Non-empty braces
+ * that aren't a {@code String.format} spec (e.g. {@code {n}}) are left
intact, and any unconsumed args are appended
+ * as {@code [arg]} — keep call sites strict if accidental drift
matters. Invalid {@code {%fmt}} specs fall back to
+ * {@code String.valueOf(arg)} so a log call never throws.
+ */
+ public static String format(String pattern, Object... args) {
Review Comment:
Is there not a Java String formatter ? and is it not as fast as this ?
##########
src/main/java/org/apache/sysds/hops/codegen/SpoofCompiler.java:
##########
@@ -538,22 +537,21 @@ public static ArrayList<Hop> optimize(ArrayList<Hop>
roots, boolean recompile)
LOG.warn("CPlan " +
tmp.getValue().getVarname() + " not supported by SPOOF CUDA");
}
- //explain debug output cplans or
generated source code
- if( LOG.isInfoEnabled() ||
DMLScript.EXPLAIN.isHopsType(recompile) ) {
- LOG.info("Codegen EXPLAIN
(generated cplan for HopID: " + cplan.getKey() +
- ", line
"+tmp.getValue().getBeginLine() + ", hash="+tmp.getValue().hashCode()+"):");
-
LOG.info(tmp.getValue().getClassname()
- +
Explain.explainCPlan(cplan.getValue().getValue()));
+ // explain debug output cplans or
generated source code
+ // (explicit guard:
Explain.explainCPlan is expensive and evaluated eagerly)
+ if(LOG.isInfoEnabled()) {
+ LOG.info("Codegen EXPLAIN
(generated cplan for HopID: {}, line {}, hash={}):", cplan.getKey(),
+
tmp.getValue().getBeginLine(), tmp.getValue().hashCode());
+
LOG.info(tmp.getValue().getClassname() +
Explain.explainCPlan(cplan.getValue().getValue()));
}
- if( LOG.isInfoEnabled() ||
DMLScript.EXPLAIN.isRuntimeType(recompile) ) {
- LOG.info("JAVA Codegen EXPLAIN
(generated code for HopID: " + cplan.getKey() +
- ", line
"+tmp.getValue().getBeginLine() + ", hash="+tmp.getValue().hashCode()+"):");
+ if(LOG.isInfoEnabled()) {
+ LOG.info("JAVA Codegen EXPLAIN
(generated code for HopID: {}, line {}, hash={}):",
+ cplan.getKey(),
tmp.getValue().getBeginLine(), tmp.getValue().hashCode());
LOG.info(CodegenUtils.printWithLineNumber(src));
-
- if(API == GeneratorAPI.CUDA) {
- LOG.info("CUDA Codegen
EXPLAIN (generated code for HopID: " + cplan.getKey() +
- ", line
" + tmp.getValue().getBeginLine() + ", hash=" + tmp.getValue().hashCode() +
"):");
+ if(API == GeneratorAPI.CUDA) {
+ LOG.info("CUDA Codegen
EXPLAIN (generated code for HopID: {}, line {}, hash={}):",
+ cplan.getKey(),
tmp.getValue().getBeginLine(), tmp.getValue().hashCode());
LOG.info(CodegenUtils.printWithLineNumber(src_cuda));
Review Comment:
I think a small cleanup is to move the tmp.getValue outside in a variable.
Can we do that as part of the cleanup ?
Maybe even the .hashCode().
##########
src/main/java/org/apache/sysds/runtime/controlprogram/caching/CacheableData.java:
##########
@@ -1295,8 +1287,7 @@ protected void acquire (boolean isModify, boolean
restore) {
throw new DMLRuntimeException("MODIFY-MODIFY
not allowed.");
}
- if( LOG.isTraceEnabled() )
- LOG.trace("Acquired lock on " + getDebugName() + ",
status: " + _cacheStatus.name() );
+ LOG.trace("Acquired lock on {} status: {}", getDebugName(),
_cacheStatus.name());
Review Comment:
doublecheck .name is constant time and with no allocation.
##########
src/main/java/org/apache/sysds/runtime/controlprogram/caching/CacheableData.java:
##########
@@ -1076,28 +1072,27 @@ protected boolean isBlobPresent() {
*/
protected void restoreBlobIntoMemory() {
String cacheFilePathAndName = getCacheFilePathAndName();
- long begin = LOG.isTraceEnabled() ? System.currentTimeMillis()
: 0;
-
- if( LOG.isTraceEnabled() )
- LOG.trace ("CACHE: Restoring matrix... " + hashCode()
+ " HDFS path: " +
- (_hdfsFileName == null ? "null"
: _hdfsFileName) + ", Restore from path: " + cacheFilePathAndName);
-
- if (_data != null)
- throw new DMLRuntimeException(cacheFilePathAndName + "
: Cannot restore on top of existing in-memory data.");
+ long begin = LOG.currentTimeMillisIfTraceEnabled();
+
+ LOG.trace("CACHE: Restoring matrix... {} HDFS path: {}
Restore from path: {}", hashCode(), _hdfsFileName,
+ cacheFilePathAndName);
+
+ if(_data != null)
+ throw new DMLRuntimeException(
+ cacheFilePathAndName + " : Cannot restore on
top of existing in-memory data.");
try {
_data = readBlobFromCache(cacheFilePathAndName);
}
catch (IOException e) {
throw new DMLRuntimeException(cacheFilePathAndName + "
: Restore failed.", e);
}
-
- //check for success
- if (_data == null)
- throw new DMLRuntimeException (cacheFilePathAndName + "
: Restore failed.");
-
- if( LOG.isTraceEnabled() )
- LOG.trace("Restoring matrix - COMPLETED ... " +
(System.currentTimeMillis()-begin) + " msec.");
+
+ // check for success
+ if(_data == null)
+ throw new DMLRuntimeException(cacheFilePathAndName + "
: Restore failed.");
+
+ LOG.trace("Restoring matrix - COMPLETED ... {} msec.",
LOG.currentTimeMillisIfTraceEnabled() - begin);
Review Comment:
Here to be a bit pedantic, we could let the currentTimeMillisIfTraceEnabled
method take an argument called offset.
then the
```
currentTimeMillisIfTraceEnabled(long off){
if(trace)
return System.currentTimeMillis - off;
}
```
##########
src/main/java/org/apache/sysds/runtime/instructions/cp/CompressionCPInstruction.java:
##########
@@ -197,8 +196,7 @@ private void processMatrixBlockCompression(ExecutionContext
ec, MatrixBlock in,
if(LOG.isTraceEnabled())
LOG.trace(compResult.getRight());
MatrixBlock out = compResult.getLeft();
- if(LOG.isInfoEnabled())
- LOG.info("Compression output class: " +
out.getClass().getSimpleName());
+ LOG.info("Compression output class: {}",
out.getClass().getSimpleName());
Review Comment:
this allocates a string, case class again ?
##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedWorkerHandler.java:
##########
@@ -290,11 +289,8 @@ private static void printStatistics() {
}
private static void logRequests(FederatedRequest request, int
nrRequest, int totalRequests) {
- if(LOG.isDebugEnabled()) {
- LOG.debug("Executing command " + (nrRequest + 1) + "/"
+ totalRequests + ": " + request.getType().name());
- if(LOG.isTraceEnabled())
- LOG.trace("full command: " +
request.toString());
- }
+ LOG.debug("Executing command {}/{}: {}", nrRequest + 1,
totalRequests, request.getType().name());
Review Comment:
Doublecheck method is constant time and do not allocate
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
