Skip to content

Conversation

@taooceros
Copy link
Member

We don't need the Lazy instance to implement the behavior, which will remove some overhead.

@jjw24
Copy link
Member

jjw24 commented May 23, 2021

Have you tested that this is the case? Thought it was used to allow faster loading

@taooceros
Copy link
Member Author

Have you tested that this is the case? Thought it was used to allow faster loading

The behavior should not change. I just implement the same logic without using Lazy

@taooceros taooceros added this to the 1.8.0 milestone May 23, 2021
@taooceros
Copy link
Member Author

I found a bug that when image is modifying quickly, it would be possible to trigger removing twice, which will result exception (index out of bound). Therefore, I add a small lock-liked mechanism to avoid that.

@jjw24
Copy link
Member

jjw24 commented May 23, 2021

ran into this error in debug:

image

@jjw24 jjw24 added the enhancement New feature or request label May 23, 2021
@taooceros
Copy link
Member Author

ran into this error in debug:

image

hmmmm I am unable to reproduce it. Would you please share where is it throw? The current stacktrace actually not working well because of error rethrow.

@jjw24
Copy link
Member

jjw24 commented May 24, 2021

The current stacktrace actually not working well because of error rethrow.

Agree, will need to rework this error handling at some point in the future.

hmmmm I am unable to reproduce it

Will take another look later to see if it's related to my environment

@jjw24
Copy link
Member

jjw24 commented Jun 29, 2021

can we leave this for after 1.8.0?

@taooceros
Copy link
Member Author

can we leave this for after 1.8.0?

Sure, it's a minor change.

@taooceros taooceros modified the milestones: 1.8.0, 1.9.0 Jun 29, 2021
@jjw24 jjw24 modified the milestones: 1.8.1, 1.9.0 Jul 26, 2021
@taooceros taooceros requested a review from jjw24 July 31, 2021 08:25
@jjw24 jjw24 added the review in progress Indicates that a review is in progress for this PR label Aug 3, 2021
@jjw24
Copy link
Member

jjw24 commented Aug 3, 2021

havent looked at the code yet, just testing it so far and i am finding the speed of results and images are a lot faster 👍

@taooceros
Copy link
Member Author

havent looked at the code yet, just testing it so far and i am finding the speed of results and images are a lot faster 👍

That's interesting. I don't expect significant performance improvement for this pr, but just a small optimization and fix a potential logic issue.

@taooceros
Copy link
Member Author

taooceros commented Aug 5, 2021

@pc223 Do you want to take a try whether this combine with #554 can resolve the freeze in debugger? Probably previously the slowness when debugger attached is because of the exception thrown when resizing imagecache.

#554 is a simple change. Just change these few lines to achieve the same effect in #554.

if (newItems.Count < 100)
{
if (Count != 0) RemoveAll(newItems.Count);
AddAll(newItems);
editTime++;
return;
}

@pc223
Copy link
Contributor

pc223 commented Aug 5, 2021

@pc223 Do you want to take a try whether this combine with #554 can resolve the freeze in debugger? Probably previously the slowness when debugger attached is because of the exception thrown when resizing imagecache.

Sure, I'll try this out and merge with #554 see what'll happens :D

@pc223
Copy link
Contributor

pc223 commented Aug 7, 2021

@pc223 Do you want to take a try whether this combine with #554 can resolve the freeze in debugger? Probably previously the slowness when debugger attached is because of the exception thrown when resizing imagecache.

#554 is a simple change. Just change these few lines to achieve the same effect in #554.

if (newItems.Count < 100)
{
if (Count != 0) RemoveAll(newItems.Count);
AddAll(newItems);
editTime++;
return;
}

Just tested, still the same behavior, seems this PR not related to the delay. Without #554, it freeze even with Release build no debugger.

@taooceros
Copy link
Member Author

If with #554, will flow freeze when debugger attached?

@pc223
Copy link
Contributor

pc223 commented Aug 7, 2021

If with #554, will flow freeze when debugger attached?

yes

@taooceros
Copy link
Member Author

Ok, so probably it is some other reason I don't fully understand😂

@pc223
Copy link
Contributor

pc223 commented Aug 7, 2021

Ok, so probably it is some other reason I don't fully understand😂

Yeah, #554 is our only clue right now, still related to the number of items being updated at once I suppose.

@pc223
Copy link
Contributor

pc223 commented Aug 7, 2021

@jjw24 can you reproduce the delay even with #554? I can't reproduce the delay if I build Release config No debugger.

I still think we should push #554 to release, it solves the delay on my machine and @kubalav

@kubalav
Copy link
Contributor

kubalav commented Aug 7, 2021

In my case it isn't working with enabled Program plugin.

@taooceros
Copy link
Member Author

Yeah I will push #554.

@taooceros
Copy link
Member Author

I fix an error thrown causing debugger slow. @pc223 Would you please check whether combining #554 still reproduce the slowness when debugger attached?

@taooceros
Copy link
Member Author

taooceros commented Aug 8, 2021

Weird.....Even with 15000 items, I won't encountered significant slowness at first run without #554.

@jjw24
Copy link
Member

jjw24 commented Aug 8, 2021

@jjw24 can you reproduce the delay even with #554? I can't reproduce the delay if I build Release config No debugger.

I have never been able to repro the delay in release or debug build. Only experiencing it in debug when I am running a virtual machine on my laptop but that's because the cpu and memory consumption is been eaten up by the VM.

@jjw24
Copy link
Member

jjw24 commented Aug 8, 2021

do we want to continue to merge this in so it's quicker to test pr 554 or is there some more changes you want to add @taooceros ?

Maybe we can get this in with release 1.8.2

@taooceros
Copy link
Member Author

do we want to continue to merge this in so it's quicker to test pr 554 or is there some more changes you want to add @taooceros ?

Maybe we can get this in with release 1.8.2

We can get this in release 1.8.2 because it does fix some logic issue. I don't have more change currently.

@jjw24
Copy link
Member

jjw24 commented Aug 8, 2021

Sounds will take a look at code soon and merge into 1.8.2 instead

@jjw24 jjw24 modified the milestones: 1.9.0, 1.8.2 Aug 8, 2021
@jjw24 jjw24 added bug Something isn't working and removed enhancement New feature or request labels Aug 8, 2021
@jjw24 jjw24 merged commit 5aa7412 into dev Aug 8, 2021
@jjw24 jjw24 deleted the imageOptimize branch August 8, 2021 22:52
@jjw24 jjw24 removed the review in progress Indicates that a review is in progress for this PR label Aug 8, 2021
@pc223
Copy link
Contributor

pc223 commented Aug 9, 2021

I fix an error thrown causing debugger slow. @pc223 Would you please check whether combining #554 still reproduce the slowness when debugger attached?

Just test with the new commits of this PR, without #554, still delay, with and without debugger.

Combine this PR and #554, same behavior as before, delay with debugger, not delay without debugger.

@jjw24 jjw24 mentioned this pull request Aug 10, 2021
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.

6 participants