Modify

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#14043 closed enhancement (fixed)

Error-prone versus PMD on usage of short type

Reported by: Don-vip Owned by: team
Priority: normal Milestone: 16.12
Component: Core Version:
Keywords: performance memory Cc: simon04, bastiK, michael2402, GerdP

Description

r11294 addresses the new error-prone warning, thanks.

But it now raises warnings from PMD, see on SonarQube.

Java uses the short type to reduce memory usage, not to optimize calculation. On the contrary, the jvm does not has an arithmetic capabilities with the type short. So, the P-code must convert the short into int, then do the proper calculation and then again, convert int to short. So, use of the "short" type may have a great effect on memory usage.

One of the two warnings must be disabled or a new solution to be found. I have no idea what's the best solution between short and int. Do we have some profiling info?

Attachments (1)

flags.patch (5.3 KB) - added by GerdP 2 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 2 years ago by GerdP

I'd try to use constants, e.g.
protected static final short FLAG_MODIFIED = 0x0001;
instead of
protected static final short FLAG_MODIFIED = 1 << 0;

comment:2 Changed 2 years ago by michael2402

We only have one short field (flags).

Probably every JVM will tend to align the other fields to 4 Byte boundaries, so the 2 bytes we save using a short are used as padding for the next int field. We don't really gain anything from it. I'd prefer simply changing the flags to int. They should not be used directly by plugins.

Currently it is volatile to support checks like "is the object disabled or incomplete" sort of atomically. We should add a comment explaining this. isDisabledAndHidden does it the wrong way by accessing the field twice.

comment:3 Changed 2 years ago by bastiK

I'd rather change OsmPrimitive.mappaintCacheIdx to short. A saving of 4 bytes per primitive is worthwhile.

comment:4 Changed 2 years ago by Don-vip

In 11368/josm:

see #14043 - review use of short type

comment:5 Changed 2 years ago by GerdP

Please review the constants in OsmPrimitive, if I got that right you should use 0x0001, 0x0002, 0x0004 etc.
The current code seems to be wrong.

comment:6 Changed 2 years ago by GerdP

I suggest to define all constants which are used with the field flags in the same source.

comment:7 Changed 2 years ago by Don-vip

In 11369/josm:

see #14043 - restore short constants to their correct value

comment:8 Changed 2 years ago by Don-vip

You're right, rookie mistake... It did not change the warning in any way so I reverted the change. I'm going to build a new latest release to avoid data corruption.

comment:9 Changed 2 years ago by Don-vip

Resolution: fixed
Status: newclosed

I have disabled the PMD warning on SonarQube.

comment:10 in reply to:  6 Changed 2 years ago by bastiK

Replying to Gerd Petermann <gpetermann_muenchen@…>:

I suggest to define all constants which are used with the field flags in the same source.

Yes, can you make a patch? :)

Changed 2 years ago by GerdP

Attachment: flags.patch added

comment:11 Changed 2 years ago by GerdP

The patch combines the constants in AbstractPrimitive.

comment:12 Changed 2 years ago by bastiK

In 11373/josm:

see #14043 - move all flags constants to AbstractPrimitive (patch by Gerd Petermann)

comment:13 Changed 2 years ago by Don-vip

Milestone: 16.1116.12

Milestone renamed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.