-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Move Duplicate Code for Perf Graphs to Common Utils Library #118
Labels
bug
Something isn't working
Projects
Comments
FYI @jiayaolinn @joransiu It would be easier to add graphs for AcmeAir if we can clean up this code. |
I think it is easier to complete this issue after #106 as the data structure is updated. |
Closed
As discussed, we break this issue into several parts. The first couple of tasks:
|
dhlee49
added a commit
to dhlee49/openjdk-test-tools
that referenced
this issue
Apr 22, 2020
related to adoptium#118,adoptium#223 Signed-off-by: Dong Hoon Lee <l.donghoon49@gmail.com>
dhlee49
added a commit
to dhlee49/openjdk-test-tools
that referenced
this issue
May 1, 2020
related to adoptium#118,adoptium#223 Signed-off-by: Dong Hoon Lee <l.donghoon49@gmail.com>
dhlee49
added a commit
to dhlee49/openjdk-test-tools
that referenced
this issue
May 6, 2020
related to adoptium#118,adoptium#223 Signed-off-by: Dong Hoon Lee <l.donghoon49@gmail.com>
dhlee49
added a commit
to dhlee49/openjdk-test-tools
that referenced
this issue
May 6, 2020
related to adoptium#118,adoptium#223 Signed-off-by: Dong Hoon Lee <l.donghoon49@gmail.com>
dhlee49
added a commit
to dhlee49/openjdk-test-tools
that referenced
this issue
May 6, 2020
related to adoptium#118,adoptium#223 Signed-off-by: Dong Hoon Lee <l.donghoon49@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Problem Description
Currently, we have 3 Perf widgets for Dashboard: DayTrader, ODM and SPECjbb2015. There is significant duplication of code between those 3 widgets since we just copied and modified the code from the first widget instead of using a common library while adding a new widget every time.
All perf graphs have the same purpose of displaying perf results for different benchmarks run on different platforms. Besides some specific data, everything else is the same among those widgets as shown below in the screenshots.
The graphs would have some minor feature difference because some of the features added by #84, where not extended to all 3 perf widgets.
As a result, it's not easy to add new widgets for new benchmarks without duplicating code from some existing widget.
Proposed Changes
We should use a library to keep the common code, in order to avoid duplicating code for different benchmark widgets. For example, currently,
utils.js
just has one functionparseSHA
, which is used in multiple widgets but there is still enough scope to clean up code by moving common code to this library.https://github.com/AdoptOpenJDK/openjdk-test-tools/blob/524d16e3784c17f4af6cee75f9105eb929397792/test-result-summary-client/src/Dashboard/Widgets/Graph/utils.js#L1-L2
https://github.com/AdoptOpenJDK/openjdk-test-tools/blob/524d16e3784c17f4af6cee75f9105eb929397792/test-result-summary-client/src/Dashboard/Widgets/Graph/ODM.jsx#L159-L160
https://github.com/AdoptOpenJDK/openjdk-test-tools/blob/524d16e3784c17f4af6cee75f9105eb929397792/test-result-summary-client/src/Dashboard/Widgets/Graph/DayTrader3.jsx#L151-L152
The text was updated successfully, but these errors were encountered: