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

Infinite loop (in scheduler?) #86

Closed
mindplay-dk opened this issue Oct 19, 2019 · 16 comments
Closed

Infinite loop (in scheduler?) #86

mindplay-dk opened this issue Oct 19, 2019 · 16 comments

Comments

@mindplay-dk
Copy link
Contributor

(You said in #84 you thought this problem was fixed, but I still have this issue with version 1.8.7 - I'm opening a separate issue to avoid getting sidetracked in other issues.)

I've created a preliminary benchmark, which sometimes causes Chrome to lock up:

image

The only way out is to kill the browser tab via Chrome's task manager.

To recreate the problem, be sure to build the fre benchmark with npm run build-prod - for some reason, the problem doesn't seem to appear with npm run build-dev, not sure (?)

Click on the button to create 10,000 rows - then click repeatedly on the "update" and "swap" buttons, which seem most likely to trigger the problem.

@yisar
Copy link
Collaborator

yisar commented Oct 19, 2019

I debugged the benchmark, there are two reasons:

  1. Scheduling problem. Our scheduling is to execute as many tasks as possible in the idle time, so when there are 10000 tasks, they will loop too many times.

  2. Diff algorithm. Because swap is the replacement of 9999 and 2, according to our current diff algorithm, 9998 inserts will be performed, which will cause browser crash.

@mindplay-dk
Copy link
Contributor Author

I looked around, and there's not much to go on - but I did find this:

vuejs/vue@850555d#diff-9ddd49414faddc237ff7c47aedcb12ff

It looks like Vue also had problems with MessageChannel and switched to MutationObserver instead?

The commit log says this fixed a number of problems:

image

Here's the current version on their dev-branch:

https://github.com/vuejs/vue/blob/dev/src/core/util/next-tick.js

Maybe this is helpful? That's all I could find.

@yisar
Copy link
Collaborator

yisar commented Oct 21, 2019

I'm back. I fixed this bug now.

The behavior of fre is now similar to react. When the task has expired, it will render synchronously.

But I made a buffer. It should expire more slowly than react.

@yisar yisar closed this as completed in f153693 Oct 21, 2019
@mindplay-dk
Copy link
Contributor Author

Same problem as before, sorry.

It's not random - I have exact steps to recreate the problem:

  • "Create 1,000 rows"
  • "Append 1,000 rows" x 3
  • "Clear"
  • "Create 10,000 rows"
  • "Update every 10th row" x 3
  • "Swap rows" 💣

It consistently freezes up moments after the last click on "Swap Rows".

You actually see the log message in the console before it freezes - and there's a moment after that where the browser is still responsive: table rows still highlight if you move the mouse over the table immediately after pressing "Swap Rows".

My guess is the loop somehow doesn't end after "Swap Rows" - it keeps going for some reason... I'm betting you could set a breakpoint in exactly the right place, you'd be able to see what it's doing after it completes the "Swap Rows" operation?

@yisar
Copy link
Collaborator

yisar commented Oct 22, 2019

Today's research results, I have found the cause of memory leaks, which from dom and reconciler.I still need some time.

@mindplay-dk
Copy link
Contributor Author

I wish I could be of more help, but I honestly don't understand all the details. 😌

yisar added a commit that referenced this issue Oct 22, 2019
@yisar
Copy link
Collaborator

yisar commented Oct 22, 2019

Good news, I have fixed this bug now. You can check the latest code.

Now I'm trying to explain why.

When we remove the elements, I find that we will skip the updates of buttons, which causes the events of buttons to not be removed, and eventually leads to memory leakage.

Now it works well, including scheduler and reconciler.

@mindplay-dk
Copy link
Contributor Author

I'm sorry, I haven't had a chance to look at your work in the past few days.

I just checked current master with the benchmark, and I'm afraid it doesn't work at all now - pressing the create or append buttons just adds a single broken-looking table row, and there are no errors in the console...

image

I don't know how come the tests are passing? (The scheduler of course works differently under test, because planWork uses a different implementation under test, I don't know if maybe that affects it?)

It looks you introduced a call to setTimeout in testUpdates - I'm not sure why. It's really hard to track what else has changed in the tests because code was reformatted and so many changes were made and then removed again, so I don't know what else was changed... Maybe consider rolling back the test to see if an older version of it still passes?

@yisar
Copy link
Collaborator

yisar commented Oct 25, 2019

In the benchmark, only one line was added because I used a while loop for commitWork, which may not save variables in the stack. I have no idea for it now.

You can change your code here, move it inside the Component, such as here

Also you can check the benchmark demo, It works well.

And for the tests, I only change the testUpdates with setTimeout 0, because useEffect now is not run after DOM operation, It is more like useLayoutEffect, For now, I use setTimeout to ensure that it always executes after DOM operation.

@mindplay-dk
Copy link
Contributor Author

Also you can check the benchmark demo, It works well.

The one in your demo folder?

I'm talking about Krause's benchmark - and it looks similar to the one in your demo folder, it just doesn't work anymore. This is the one I've been testing with. I found the crashing/freezing issues while running this. Those issues aside, it was able to render before, and now doesn't work at all.

Maybe try it out and see what you make of it? I don't know why it shouldn't work. There are not errors in the console when pressing the "create rows" button, it just doesn't work anymore.

For now, I use setTimeout to ensure that it always executes after DOM operation.

Would it make more sense to just use useLayoutEffect in testUpdates? It should work as before then?

@yisar
Copy link
Collaborator

yisar commented Oct 25, 2019

  1. The reason why benchmark doesn't work is that I refactor commitwork. To save memory, I use the while loop, so there is no way to save external variables in the stack, so you need to move the external JSX to the internal component.

  2. Before I implement the async useeffect, I must use setTimeout to ensure that I get the correct result after DOM operation.

@yisar
Copy link
Collaborator

yisar commented Oct 25, 2019

Look at this line, this is why benchmark not work, just move it inner component.
https://github.com/132yse/fre/blob/master/demo/benchmark/src/main.jsx#L105
Due to network, I can't clone this project and can't provide PR, but it's just the difference in this line. Stare at it.

@yisar
Copy link
Collaborator

yisar commented Oct 25, 2019

Hey, I created a PR, please check it: mindplay-dk/js-framework-benchmark#1

@yisar
Copy link
Collaborator

yisar commented Oct 26, 2019

I've fixed all bugs now. I hope this is the last time. benchmark is works well now.

@mindplay-dk
Copy link
Contributor Author

Benchmark confirmed: no stalling or freezing, produces expected results! 😀🎉

I do believe this introduced a new issue with useEffect - but lets regard this issue as closed, and I will open a separate issue for that.

Good work! 🙂

@yisar
Copy link
Collaborator

yisar commented Oct 26, 2019

The problem of useeffect is here: #87

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

No branches or pull requests

2 participants