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

JBPM 8836 : Improve Performance of Copy/Cut and Paste Operations #2983

Merged
merged 9 commits into from Nov 26, 2019

Conversation

inodeman
Copy link
Contributor

@inodeman inodeman commented Oct 29, 2019

JBPM 8836 : Improve Performance of Copy/Cut and Paste Operations
JBPM-8835 : Improve performance of response when user performs a click
JBPM-8806 : Stunner - Cursor movement is not smooth
JBPM-8872 : Improve Realtime Drawing When Dragging / Moving Shapes
JBPM-8891: Improve Diagram Loading Times
JBPM-8892: Performance Degradation for Large Models
JBPM-8873 : Adding Nodes to the Canvas is Slow
JBPM-8870 : Stunner - Toolboxes seem to be destroyed and redrawn each time you select the same node
JBPM-8874 : Stunner - Prevent unnecessary instances of Copy, Delete Commands to e created by CDI.
JBPM-8877 : Improve performance of Moving Shapes with Keys
JBPM-8879 : Stunner - Performing Containment takes too long
JBPM-8881 : Stunner - Catching intermediate events are displayed slower compared to other nodes
JBPM-8181 : Stunner - Events docking is very slow when Diagram properties is opened
JBPM-8817 : Prevent Unnecesary Listeners to get added everytime a user performs a click.

@inodeman
Copy link
Contributor Author

Hi @romartin @hasys this is the Performance PR, please take a look and give feedback, I will work on a spreadsheet that contains the previous times and current times.

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@hasys
Copy link
Member

hasys commented Oct 29, 2019

Hi @inodeman, sonar cloud gate is failed, and some issues looks quite relevant to me. For example missing asserts in tests and possible race condition. It is worth for investigations. Thank you!

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

1 similar comment
@inodeman
Copy link
Contributor Author

Jenkins retest please

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@hasys
Copy link
Member

hasys commented Oct 30, 2019

Hi @inodeman,

now Sonar is happy, but some tests failing on Jenkins:

org.kie.workbench.common.stunner.core.client.session.command.impl.CopySelectionSessionCommandTest.testExecute org.kie.workbench.common.stunner.core.client.session.command.impl.DeleteSelectionSessionCommandTest.testClearSessionInvoked org.kie.workbench.common.stunner.core.client.session.command.impl.DeleteSelectionSessionCommandTest.testHandleCanvasSelectionEventWhenElementsAreSelected org.kie.workbench.common.stunner.core.client.session.command.impl.DeleteSelectionSessionCommandTest.testHandleCanvasClearSelectionEvent org.kie.workbench.common.stunner.core.client.session.command.impl.DeleteSelectionSessionCommandTest.testHandleCanvasElementsClearEvent org.kie.workbench.common.stunner.core.client.session.command.impl.DeleteSelectionSessionCommandTest.testHandleCanvasSelectionEventWhenCanvasRootIsSelected org.kie.workbench.common.stunner.core.client.session.command.impl.DeleteSelectionSessionCommandTest.checkRespondsToExpectedKeys org.kie.workbench.common.stunner.core.client.session.command.impl.DeleteSelectionSessionCommandTest.checkDoesNotRespondToExpectedKeysWhenDisabled org.kie.workbench.common.stunner.core.client.session.command.impl.DeleteSelectionSessionCommandTest.checkBindAttachesKeyHandler org.kie.workbench.common.stunner.core.client.session.command.impl.DeleteSelectionSessionCommandTest.checkDoesNotRespondToOtherKey

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@hasys
Copy link
Member

hasys commented Nov 1, 2019

Hi @inodeman,

master branch and jenkins builds are broken due to #2989
Let's try again when mentioned PR will be merged. Thank you!

@hasys
Copy link
Member

hasys commented Nov 5, 2019

Jenkins retest please

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@hasys
Copy link
Member

hasys commented Nov 18, 2019

Hi @inodeman,

so far looks good, but I found one regression. When I am adding several nodes from the pallet starting with the second node you need to perform several clicks on the canvas to add the node.

Steps To Reproduce

  • Create new process
  • Using palette add one any node
  • Using palette try to add one more node:
    • Click on palette category to open it
    • Click on any node in that category
    • Move mouse somewhere on canvas
    • Click left mouse button one more time

Actual Result

Node won't be added, you need to click on canvas 1-2 more times to add the node.

Expected Result

One click should be enough to add the node to the canvas.

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@inodeman
Copy link
Contributor Author

@hasys ?@romartin can you re-check this PR, Roger, I had to revert back the use of Timer instead of Deferred Command as it was causing draw delays on Macs

Copy link

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

Just out of interest I briefly checked the code and commented one line.

Comment on lines 197 to 198
final List<Element> updatedElements = new ArrayList<>();
updatedElements.add(mock(Element.class));
Copy link

Choose a reason for hiding this comment

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

As this is single element list we could use:

final List<Element> updatedElements = Collections.singletoneList(mock(Element.class));

@hasys
Copy link
Member

hasys commented Nov 20, 2019

Hi @inodeman,

sorry, but I found a blocker. When I am open two processes at the same time, first process starts to be unusable, if you select and move any node you will see this:

image

I can't reproduce this issue on Master branch, so it should be caused by this PR. Please update this PR and PR for 7.30 branch as well, thank you!

Steps To Reproduce

*Create Process A

  • Create Process B
  • Open both processes at the same time (use breadcrumbs to switch from process back to library without closing the process)
  • move nodes in process B, switch to process A (using process name as on picture below) and move any node again
    image

Actual Result

Error will be shown on any move of any node in the process you opened as first.

Expected Result

You should be able to open any amount of processes at the same time as you wish.

@inodeman
Copy link
Contributor Author

Jenkins execute full downstream build

@inodeman
Copy link
Contributor Author

Jenkins retest please

@inodeman
Copy link
Contributor Author

inodeman commented Nov 20, 2019

Hi @jomarko changed that line in latest commit. @hasys fixed, seems like with the commit to show the properties on startup somehow the forms renderer was sometimes set to null which conflicted with some code. @romartin ready for review
image
thats why the issue is only visible with both commits but not separate

@inodeman
Copy link
Contributor Author

hi @jomarko checking on your review, @romartin needs your review approval to merge it, let me know

Copy link

@jomarko jomarko left a comment

Choose a reason for hiding this comment

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

Checked Copying DMN nodes, GDT cells, seems working.

@romartin romartin merged commit f15bf48 into kiegroup:master Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants