Skip to content

Memoize ConsoleLine#827

Merged
timja merged 1 commit intojenkinsci:mainfrom
basil:memo
Jun 5, 2025
Merged

Memoize ConsoleLine#827
timja merged 1 commit intojenkinsci:mainfrom
basil:memo

Conversation

@basil
Copy link
Member

@basil basil commented Jun 5, 2025

A minor performance improvement, not enough to fix fully #823, but enough to be measurable in profiling so I thought I would submit it. See React documentation.

Testing done

With

diff --git a/src/main/frontend/pipeline-console-view/pipeline-console/main/ConsoleLine.tsx b/src/main/frontend/pipeline-console-view/pipeline-console/main/ConsoleLine.tsx
index 7db8a20..87aefab 100644
--- a/src/main/frontend/pipeline-console-view/pipeline-console/main/ConsoleLine.tsx
+++ b/src/main/frontend/pipeline-console-view/pipeline-console/main/ConsoleLine.tsx
@@ -29,6 +29,8 @@ export const ConsoleLine = (props: ConsoleLineProps) => {
     );
   }, []);
 
+  console.log(`Rendering console line ${props.stepId}-${props.lineNumber}`);
+
   return (
     <pre
       style={{ background: "none", border: "none" }}

I saw that previous lines were being re-rendered every time a new line appeared. With this PR, only new lines are rendered, which drastically reduced the number of log statements I saw from thousands to several hundred.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

@basil basil requested a review from a team as a code owner June 5, 2025 15:03
@basil basil added the bug Something isn't working label Jun 5, 2025

// Console output line
export const ConsoleLine = (props: ConsoleLineProps) => {
export const ConsoleLine = memo(function ConsoleLine(props: ConsoleLineProps) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After wrapping the component with memo, the resulting component's name was not automatically inferred, causing ESLint to flag a "missing displayName" error. By defining the component with a named function rather than the arrow syntax, React infers the displayName automatically.

Copy link
Contributor

@felipecrs felipecrs Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. That's the exact same thing I did here:

image

@timja
Copy link
Member

timja commented Jun 5, 2025

(edited your description to not cause it to be closed on merge, wording isn't the best but otherwise GitHub would close it)

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally looks great thanks.

I thought it might fix #348 but it doesn't 😢.

@timja timja merged commit 102a888 into jenkinsci:main Jun 5, 2025
17 checks passed
@basil basil deleted the memo branch June 5, 2025 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants