Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 336211

Summary:

[prefs] ProjectPreferences#initialize overwrites child nodes unnecessarily

Product:

[Eclipse Project] Platform

Reporter:

Szymon Ptaszkiewicz <sptaszkiewicz>

Component:

Resources

Assignee:

Szymon Ptaszkiewicz <sptaszkiewicz>

Status:

RESOLVED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

dj.houghton, john.arthorne, Szymon.Brandys, tobiasbertelsen

Version:

3.7

Target Milestone:

4.4 M3

Hardware:

All

OS:

All

Whiteboard:

Attachments:

Description	Flags
Patch v.0.1	none
Patch v.0.2	none

Description Szymon Ptaszkiewicz

2011-02-03 05:38:19 EST

Created attachment 188215 [details]
Patch v.0.1

In the fix for bug 335591 we have added additional initialization in #load. This fixed the problem however resulted in subsequent child node creation and unnecessary preferences loading. We should avoid unnecessary node creation because of performance reasons. The same applies to subsequent loads because #load reads data from disk which can be quite expensive.

What we could do is to postpone loading until the project node is initialized. To get correct preference value, the following preconditions would have to be met:
1. Project is open.
2. Project node is initialized. This can be done if point 1 is satisfied by creating project node or by calling directly #initialize on already created project node.
3. Load level node is loaded. This can be done if point 2 is satisfied by calling #load during node creation, by calling #read when prefs file content has changed or by trying to access some preference stored in this node for the first time (#internalGet).

Attached patch shows the initial approach.

Comment 1 Szymon Ptaszkiewicz

2011-02-07 11:12:40 EST

Created attachment 188448 [details]
Patch v.0.2

Comment 2 Szymon Ptaszkiewicz

2011-10-18 07:52:35 EDT

DJ, what do you think about the above enhancement?

Comment 3 DJ Houghton

2011-10-18 16:14:11 EDT

I'm not sure I quite understand the use case and what is happening. Can you explain it in a bit more detail? I thought that the old code only loaded nodes at the load level, but from this bug description I guess that isn't the case anymore?

The attached test case has an example of setting project-level preference values for closed projects. Is this a scenario that we want to even support?

Comment 4 Szymon Ptaszkiewicz

2011-11-01 09:27:42 EDT

(In reply to comment #3)
> I'm not sure I quite understand the use case and what is happening.

I think the patch can be misleading because it mixes at least two different ideas: lazy loading and removing unnecessary node creation if the node is already created. I should split it into separate bugs to avoid confusion.

> Can you
> explain it in a bit more detail? I thought that the old code only loaded nodes
> at the load level, but from this bug description I guess that isn't the case
> anymore?

Loading is still done at the load level but in the patch it is postponed until the first use (set or get) of this node occurs. After second thought, I am not sure that this is a big improvement and is really needed. Preference files are not that big and usually when we create a node we try to set or get a value immediately.

> The attached test case has an example of setting project-level preference
> values for closed projects. Is this a scenario that we want to even support?

This is allowed to set project-level preference values for closed projects without storing the node on disk. It is not a good pattern but it is possible.

Comment 5 Szymon Brandys

2011-11-04 07:40:35 EDT

Szymon P., as you said the patch tries to address multiple issues. Let's split the bug into smaller ones and discuss them one by one. Could you tell what the performance gain is when the lazy loading is applied?

Comment 6 Szymon Ptaszkiewicz

2013-08-23 08:35:54 EDT

*** Bug 415750 has been marked as a duplicate of this bug. ***

Comment 7 Szymon Ptaszkiewicz

2013-10-15 09:16:15 EDT

As mentioned in comment 4, the original patch handled two issues: lazy loading and removing unnecessary node creation if the node was already created. I will use this bug to fix the removing unnecessary node creation part. Lazy-loading is not backed by any performance measures so I will drop this idea for now until we get some better rationale to implement it.

Comment 8 Szymon Ptaszkiewicz

2013-10-15 09:47:59 EDT

Fixed in master:
http://git.eclipse.org/c/platform/eclipse.platform.resources.git/commit/?id=f71edcf4c3eb45810bee0ab883e34327fac33499

Tobias, since you found a real problem in your product because of this bug, I'd appreciate if you could verify if the fix solves your problem. Thanks in advance!

Comment 9 Tobias Bertelsen

2013-10-15 11:07:45 EDT

In my setup it works fine now :)