Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client: Use ArrayLists instead of Vectors #120

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

StenAL
Copy link

@StenAL StenAL commented Sep 1, 2024

ArrayLists come with a nice 20-30% performance boost and are the preferred way of dealing with ordered collections in modern Java, see https://stackoverflow.com/q/2986296 for more information.

Unlike Vectors, they are not internally synchronized which means the code has to be audited for structural modifications to the ArrayLists and those places have to be explicitly synchronized if they are called by multiple threads.

For each class that used Vectors, I checked that it is safe to convert them to ArrayLists (i.e. all methods that modify the collection that can be called from multiple threads are synchronized):

  • AdCanvas -- texts is only modified in the create method, which is only called once from run in AApplet so there's no need for synchronization.
  • ChatBase -- added synchronization to methods that modify chatListeners.
  • Choicer -- only modifies listeners in synchronized blocks.
  • ColorButton -- only modifies listeners in synchronized blocks.
  • ColorCheckbox -- access to listeners only happens in synchronized blocks.
  • ColorCheckboxGroup -- added synchronization around addCheckbox method.
  • ColorList -- nodes is only modified in synchronized blocks. items and listeners are modified only in synchronized methods.
  • ColorSpinner -- items and listeners only modified in synchronized blocks.
  • ColorTextArea -- lines and texts are only modified in synchronized blocks.
  • Connection -- added synchronization around thriftLogs modification.
  • GameCanvas -- teleportStarts and teleportExists are only modified in init method called once for every track so there's no need for synchronization.
  • GamePacketQueue -- access to packets only happens in synchronized methods.
  • GameQueue -- added synchronization for commands modification in add method. pop is only called from the run function of Connection so it does not need synchronization.
  • HackedShot -- teleportStarts and teleportExits are never modified, no need for synchronization.
  • HtmlLine -- words is only modified from createLines in HtmlText which is only called from its constructor so there is no need for synchronization.
  • HtmlText -- lines is only modified in createLines, which is only called from the constructor, so there is no need for synchronization.
  • ImageTracker -- all access to imageResourceTable is synchronized except for removeAllImageResources which is only called once when the applet is destroyed so there's no need for synchronization.
  • InputTextField -- userInput and listeners are only modified in synchronized blocks.
  • MultiColorList -- colorListItems and listeners are only modified in synchronized methods.
  • QuickTimer -- added synchronization to the methods that modify listeners.
  • RadioButtonGroup -- added synchronization to methods that modify buttons.
  • RoundButton -- access to listeners only happens in synchronized blocks.
  • StringDraw -- Vector was not used for any state, only to store intermediate results. No need for synchronization.
  • TabBar -- listeners and items are only modified in synchronized blocks.
  • Tools -- did not modify the Vector, no synchronization needed.
  • UserList -- added synchronization to methods that modify privateMessageUsers and ignoredUsers.
  • XmlUnit -- all access to children happens in synchronized blocks.

In general, I mostly replicated the synchronization from the previous behavior with Vector which might result in some unnecessary synchronization but since the client is not performance-critical, it's not too bad to over-synchronize.

Copy link
Owner

@PhilippvK PhilippvK left a comment

Choose a reason for hiding this comment

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

Looks good! Mergind this probably in 24 hours if @pehala has no complaints.

Copy link
Collaborator

@pehala pehala left a comment

Choose a reason for hiding this comment

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

Nice patch, there is always a possibility of introducing some race condition with soemthing like this, but we can revert this if we encounter it anyway.

All of the comments are only suggestions of what else could be part of this refactor. Feel free to ignore them or implement later

@StenAL
Copy link
Author

StenAL commented Sep 7, 2024

I've incorported all of @pehala's suggestions except for the listToStringArray one where I've removed the whole method since it was unused.

ArrayLists come with a nice 20-30% performance boost and are the
preferred way of dealing with ordered collections in modern Java, see
https://stackoverflow.com/q/2986296 for more information.

Unlike Vectors, they are not internally synchronized which means the
code has to be audited for structural modifications to the ArrayLists
and those places have to be explicitly synchronized if they are called
by multiple threads.

For each class that used Vectors, I checked that it is safe to convert
them to ArrayLists (i.e. all methods that modify the collection that
can be called from multiple threads are synchronized):
- AdCanvas -- `texts` is only modified in the `create` method, which is
  only called once from `run` in AApplet so there's no need for
  synchronization.
- ChatBase -- added synchronization to methods that modify
  `chatListeners`.
- Choicer -- only modifies `listeners` in synchronized blocks.
- ColorButton -- only modifies `listeners` in synchronized blocks.
- ColorCheckbox -- access to `listeners` only happens in synchronized
  blocks.
- ColorCheckboxGroup -- added synchronization around `addCheckbox`
  method.
- ColorList -- `nodes` is only modified in synchronized blocks.
  `items` and `listeners` are modified only in synchronized methods.
- ColorSpinner -- `items` and `listeners` only modified in synchronized
  blocks.
- ColorTextArea -- `lines` and `texts` are only modified in synchronized
  blocks.
- Connection -- added synchronization around `thriftLogs` modification.
- GameCanvas -- `teleportStarts` and `teleportExists` are only modified
  in `init` method called once for every track so there's no need for
  synchronization.
- GamePacketQueue -- access to `packets` only happens in synchronized
  methods.
- GameQueue -- added synchronization for `commands` modification in
  `add` method. `pop` is only called from the `run` function of
  `Connection` so it does not need synchronization.
- HackedShot -- `teleportStarts` and `teleportExits` are never
  modified, no need for synchronization.
- HtmlLine -- `words` is only modified from `createLines` in `HtmlText`
  which is only called from its constructor so there is no need for
  synchronization.
- HtmlText -- `lines` is only modified in `createLines`, which is only
  called from the constructor, so there is no need for synchronization.
- ImageTracker -- all access to `imageResourceTable` is synchronized
  except for `removeAllImageResources` which is only called once when
  the applet is destroyed so there's no need for synchronization.
- InputTextField -- `userInput` and `listeners` are only modified in
  synchronized blocks.
- MultiColorList -- `colorListItems` and `listeners` are only modified
  in synchronized methods.
- QuickTimer -- added synchronization to the methods that modify
  `listeners`.
- RadioButtonGroup -- added synchronization to methods that modify
  `buttons`.
- RoundButton -- access to `listeners` only happens in synchronized
  blocks.
- StringDraw -- `Vector` was not used for any state, only to
  store intermediate results. No need for synchronization.
- TabBar -- `listeners` and `items` are only modified in synchronized
  blocks.
- Tools -- did not modify the `Vector`, no synchronization needed.
- UserList -- added synchronization to methods that modify
  `privateMessageUsers` and `ignoredUsers`.
- XmlUnit -- all access to `children` happens in synchronized blocks.

In general, I mostly replicated the synchronization from the previous
behavior with Vector which might result in some unnecessary
synchronization but since the client is not performance-critical, it's
not too bad to over-synchronize.
@PhilippvK PhilippvK merged commit b14125d into PhilippvK:master Sep 18, 2024
2 checks passed
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.

3 participants