Bug 573093 - Add static methods and use StackWalker to simplify creating Status
Summary: Add static methods and use StackWalker to simplify creating Status
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Components (show other bugs)
Version: unspecified   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 4.20 M3   Edit
Assignee: Jonah Graham CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
Depends on:
Blocks:
 
Reported: 2021-04-22 16:57 EDT by Jonah Graham CLA
Modified: 2021-05-23 14:38 EDT (History)
5 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jonah Graham CLA 2021-04-22 16:57:26 EDT 
Bug 561712 simplified the Status API a little by allowing an arbitrary class to be passed into the constructor to determine the containing plug-in ID.

With Java 11 now a requirement to run the platform, at some point Equinox can have the same requirement and then the StackWalker API (new to Java 9) can be adopted. This can allow callees to determine the calling class automatically.

This avoids more boiler plate code when creating Status objects, and prevents a class of copy-paste error where the wrong plug-in ID or class is passed to the Status object.

I have a patch I will submit, split into two, because once the simpler API is available, having static methods make it even simpler to use:

IStatus status = Status.warning("something to warn about");

compared to today:

IStatus status = new Status(IStatus.WARNING, MyClass.class, "something to warn about");

(FWIW - the current use cases of the new methods do what I wrote on the above line, not sure why they don't use getClass())
Comment 1 Eclipse Genie CLA 2021-04-22 18:21:05 EDT 
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/179698
Comment 2 Eclipse Genie CLA 2021-04-22 18:21:08 EDT 
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/179699
Comment 3 Eclipse Genie CLA 2021-04-22 18:21:11 EDT 
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/179700
Comment 4 Eclipse Genie CLA 2021-04-25 21:17:35 EDT 
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/179811
Comment 6 Vikas Chandra CLA 2021-04-27 08:29:27 EDT 
(In reply to Eclipse Genie from comment #5)
> Gerrit change
> https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/179811 was merged
> to [master].
> Commit:
> http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/
> ?id=06804a4bd425941257a3d6bfee0f79464ceb2503

This causes https://bugs.eclipse.org/bugs/show_bug.cgi?id=573187

Any ideas how to fix the failing tests?
Comment 7 Andrey Loskutov CLA 2021-04-28 12:02:03 EDT 
(In reply to Eclipse Genie from comment #5)
> Gerrit change
> https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/179811 was merged
> to [master].
> Commit:
> http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/
> ?id=06804a4bd425941257a3d6bfee0f79464ceb2503

That shows now API error in SDK:
Description	Resource	Path	Location	Type
The minor version should be incremented in version 3.14.100, since execution environments have been changed since version 3.14.100	MANIFEST.MF	/org.eclipse.equinox.common/META-INF	line 5	Version Numbering Problem
Comment 8 Eclipse Genie CLA 2021-04-28 12:02:14 EDT 
New Gerrit change created: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/179933
Comment 9 Andrey Loskutov CLA 2021-04-28 12:02:45 EDT 
(In reply to Eclipse Genie from comment #8)
> New Gerrit change created:
> https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/179933

That is the version bump patch.
Comment 11 Jonah Graham CLA 2021-04-30 14:01:53 EDT 
In the Gerrit, Thomas asked:
> Do we anticipate how often this new Status constructor may be used?  We need to understand if high usage of the StackWalker here will impact performance.

I did some basic experiments. The original Status constructors are very fast - not surprising, they don't do very much. The ones using the class and looking it up in the framework are about 20x slower, and the new ones using stackwalker* are about 4x slower than that.

On this oldish computer I have it looks like this:

Original constructors - 16ns
Using FrameworkUtil.getBundle - 390ns = 24x line above
Using StackWalker* - 1.8us = 4.6x line above

However all of that is small compared to how long it takes to throw an Exception. If you consider the combined throw + Status creation, the times look like:

Original constructors - 3.9us
Using FrameworkUtil.getBundle - 4.7us
Using StackWalker* - 6.4 us


*Which also uses FrameworkUtil.getBundle

So, yes there is an impact - and in some cases users will need to be careful about which method they use. I will try to add some documentation to that.

----

To get the times above I did a quick wallclock measurement by running the below in a JUnit plug-in test:

	private static final int COUNT = 10000000;
	volatile static IStatus status = null;

	public void except() throws Exception {
		throw new Exception();
	}

	public void testPlain() {
		for (int i = 0; i < COUNT; i++) {
			try {
				except();
			} catch (Exception e) {
				status = new Status(0, "id", 0, "message", e);
			}
		}
	}

	public void testClass() {
		for (int i = 0; i < COUNT; i++) {
			try {
				except();
			} catch (Exception e) {
				status = new Status(0, getClass(), "message", e);
			}
		}
	}

	public void testStackWalker1() {
		for (int i = 0; i < COUNT; i++) {
			try {
				except();
			} catch (Exception e) {
				status = Status.error("message", e);
			}
		}
	}
Comment 12 Eclipse Genie CLA 2021-04-30 15:31:45 EDT 
New Gerrit change created: https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/180075
Comment 15 Eclipse Genie CLA 2021-05-17 15:11:39 EDT 
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/180702
Comment 17 Eclipse Genie CLA 2021-05-23 14:15:50 EDT 
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/180928