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

ISPN-4400 Takes a long time to start up all clusters which has a lot of ... #2639

Closed
wants to merge 1 commit into from

Conversation

anistor
Copy link
Member

@anistor anistor commented Jun 13, 2014

...segments

This is mainly caused by the myriad of internal copies performed by the CopyOnWriteArraySet used in InboundTransferTask for tracking the segments being requested during ST. This collection is updated way too frequently if we have a large number of segments. It seems that using a simple HashMap and doing external synchronization works better both in terms of time and memory.

Jira: https://issues.jboss.org/browse/ISPN-4400

return segments;
synchronized (segments) {
Set<Integer> segmentsCopy = new HashSet<Integer>(segments);
segmentsCopy.addAll(segments);
Copy link
Member

Choose a reason for hiding this comment

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

why the addAll()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Removed it.

@pruivo
Copy link
Member

pruivo commented Jun 23, 2014

synchronization is missing in toString()

@@ -33,9 +32,15 @@
private static final Log log = LogFactory.getLog(InboundTransferTask.class);
private static final boolean trace = log.isTraceEnabled();

private final Set<Integer> segments = new CopyOnWriteArraySet<Integer>();
/**
* All access to fields {@code segments} and {@code finishedSegments} must bead done while synchronizing on {@code segments}.
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to see a new Object to synchronize, but I can live with it...

@pruivo
Copy link
Member

pruivo commented Jun 23, 2014

the rest looks good :)

@anistor
Copy link
Member Author

anistor commented Jun 26, 2014

InboundTransferTask.toString() is only used during debugging and the lack of syncronization does not induce any state corruption, it may however be a nuisance to the caller. I've added syncronization.

…of segments

This is mainly caused by the myriad of internal copies performed by the CopyOnWriteArraySet used in InboundTransferTask for tracking the segments being requested during ST. This collection is updated way too frequently if we have a large number of segments. It seems that using a simple HashMap and doing external synchronization works better both in terms of time and memory.
@pruivo
Copy link
Member

pruivo commented Jun 26, 2014

I just say that because it can throw ConcurrentModificationException during the toString(). this can cause failures in tests or similar :)

@pruivo
Copy link
Member

pruivo commented Jun 27, 2014

looks good, testing and pulling...

@pruivo
Copy link
Member

pruivo commented Jun 27, 2014

integrated! thanks @anistor !

@pruivo pruivo closed this Jun 27, 2014
@anistor anistor deleted the t_4400_m branch June 27, 2014 12:39
@anistor
Copy link
Member Author

anistor commented Jun 27, 2014

Thanks @pruivo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants