Skip to content
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

Feat: Add timing to the main log output (ref #368) #372

Conversation

ReBy298
Copy link

@ReBy298 ReBy298 commented Nov 7, 2024

Feature #368
Add Timing to the main log output

Description

  1. Start Time Capture: I introduced a startTime variable to record the current time at the beginning of the function with Date.now().

  2. Execution Duration Calculation: After the command completes, I added a duration variable to calculate the time taken by subtracting startTime from the current time (Date.now()), which gives the duration in milliseconds.

  3. Enhanced Logging: I updated the console.info log message to include the duration in milliseconds (%dms). Now, the log shows not only the completion message ("EO program assembled") but also the time it took to execute.

Modified lines:

  1. console.info('EO program assembled in %s in %dms', rel(target), duration); at assemble.js
  2. console.info('Java .class files compiled into %s in %dms', rel(target), duration); at compile.js
  3. console.info('Java sources generated in %s in %dms', rel(sources), duration); at transpile.js
  4. console.info('EO objects registered in %s in %dms', rel(foreign), duration); at register.js
  5. console.info('EO program verified in %s in %dms', rel(path.resolve(opts.target)), duration); at verify.js

PR-Codex overview

This PR adds timing measurements to various command functions in the codebase, allowing for better performance tracking by logging the duration of each operation.

Detailed summary

  • Added startTime variable to capture the start time for each command function.
  • Calculated duration by subtracting startTime from the current time.
  • Updated console log messages to include the duration in milliseconds for:
    • verify
    • assemble
    • register
    • compile
    • transpile

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@yegor256
Copy link
Member

yegor256 commented Nov 8, 2024

@ReBy298 looks very good, thanks! However, don't you think that it's an obvious code duplication? There are more places in the application that need similar time reporting. If you do the same changes in them too, you will see how many duplicate lines of code we'll get. Maybe it would be better if we introduce some sort of a wrapper (this is pseudo-code):

module.exports = function(opts) {
  const target = path.resolve(opts.target);
  elapsed((e) => {
    return mvnw(['eo:assemble'].concat(flags(opts)), opts.target, opts.batch).then((r) => {
      e.print(`EO program assembled in ${rel(target)}`);
      return r;
    };
  });
};

Here, the elapsed function is the one we create and re-use in all places, where we need to print the elapsed time.

@ReBy298
Copy link
Author

ReBy298 commented Nov 8, 2024

@ReBy298 looks very good, thanks! However, don't you think that it's an obvious code duplication? There are more places in the application that need similar time reporting. If you do the same changes in them too, you will see how many duplicate lines of code we'll get. Maybe it would be better if we introduce some sort of a wrapper (this is pseudo-code):

module.exports = function(opts) {
  const target = path.resolve(opts.target);
  elapsed((e) => {
    return mvnw(['eo:assemble'].concat(flags(opts)), opts.target, opts.batch).then((r) => {
      e.print(`EO program assembled in ${rel(target)}`);
      return r;
    };
  });
};

Here, the elapsed function is the one we create and re-use in all places, where we need to print the elapsed time.

Okey let me fix it

@ReBy298 ReBy298 closed this Nov 12, 2024
@ReBy298 ReBy298 deleted the feature/Show-timing-in-the-main-log-output branch November 12, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants