Skip to content

6052 pageload slow#6332

Closed
JayanthyChengan wants to merge 1 commit intoIQSS:developfrom
JayanthyChengan:6052-pageload-slow
Closed

6052 pageload slow#6332
JayanthyChengan wants to merge 1 commit intoIQSS:developfrom
JayanthyChengan:6052-pageload-slow

Conversation

@JayanthyChengan
Copy link
Contributor

Related to #6052

@JayanthyChengan
Copy link
Contributor Author

@pdurbin - please refer to this comment
#6052 (comment)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 19.455% when pulling 24275d2 on JayanthyChengan:6052-pageload-slow into 242b6b9 on IQSS:develop.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

@JayanthyChengan hi! Thanks for this pull request! I see you said it relates to #6052 but do you think it's also a potential fix?

@JayanthyChengan
Copy link
Contributor Author

@pdurbin - Yes, I tested the code and also released latest version at dataverse.scholarsportal.info today. Please verify the performance .

Copy link
Contributor

@poikilotherm poikilotherm left a comment

Choose a reason for hiding this comment

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

I'm sorry, but isn't the root cause then that the URLClassLoader is slow? If this fix is applied, the resource files are loaded from the default classpath. The updated docs do not reflect that. Actualy removing the loader with this patch will silently disable the entire functionality of providing a directory in that system property...

@djbrooke
Copy link
Contributor

@JayanthyChengan @poikilotherm @pdurbin It's great to see the performance boost here but it looks like there may be an unresolved question so I'll move this back to Community Dev for now.

@pdurbin
Copy link
Member

pdurbin commented Nov 1, 2019

@JayanthyChengan heads up that it's a holiday for @poikilotherm today so I wouldn't expect a reply right away. Thanks again for this pull request! If there's anything I can do to help move it forward, please let me know! A couple times at standup in recent memory @landreev has mentioned how much of a problem #6052 is so he might want to take a look when he has a few minutes.

@poikilotherm
Copy link
Contributor

poikilotherm commented Nov 5, 2019

I just made a start with profiling to get to the root cause of this issue, as I don't think we should break the feature, but instead fix the implementation.

Please see my comments on #6052 and the chatter on IRC today.

@djbrooke I will unassign me, as IMHO I created a base from where to continue. I need to get going with #5974... @JayanthyChengan: happy to answer questions. Find me on IRC from 9-17 CET, happy to videochat.

@poikilotherm poikilotherm removed their assignment Nov 5, 2019
@djbrooke
Copy link
Contributor

djbrooke commented Nov 5, 2019

Thanks @poikilotherm for your efforts here, we appreciate it!!

@JayanthyChengan, let us know if you want to discuss anything about @poikilotherm's feedback. I'll keep this in community dev for now.

@pdurbin
Copy link
Member

pdurbin commented Dec 4, 2019

This morning I pinged @JayanthyChengan and she mentioned at #6052 (comment) that in production they use the workaround that this pull request represents.

She is busy with large file upload (Canarie Project) so I'll unassign her.

It probably doesn't make sense to have this in "community dev" right now. @djbrooke should we move it to another column?

@pdurbin
Copy link
Member

pdurbin commented Dec 6, 2019

@JayanthyChengan now that pull request #6428 has been merged, is it ok to close this pull request?

@mreekie mreekie added the bk2211 label Nov 1, 2022
@pdurbin
Copy link
Member

pdurbin commented Jan 4, 2023

@JayanthyChengan Happy New Year! I'm just checking in.

Above I asked if this PR can be closed. Please advise!

Also, you may be aware of that we're hoping to restructure Dataverse ( https://groups.google.com/g/dataverse-community/c/uDeNUfqe3RE/m/dph8fZUaEAAJ ) and I'm wondering if this PR will still apply if we move from JSF to React. I'm not sure, to be honest! 😄

@JayanthyChengan
Copy link
Contributor Author

@pdurbin - I am closing this now as we are not seeing any issues with page load at ScholarsPortal. Thanks

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

Successfully merging this pull request may close these issues.

6 participants