Skip to main content

Poor Performance question

8 replies [Last post]
devuser007
Offline
Joined: 2009-01-17
Points: 0

Hi Friends,

I came across this question and was unable to answer this. Can anyone help me regarding this question?

This is the given Class

public class SlowDictionary {
private final Map dict = new HashMap();

public synchronized String translate(String word)
throws IllegalArgumentException {
if (!dict.containsKey(word)) {
throw new IllegalArgumentException(word + " not found.");
}
return dict.get(word);
}

public synchronized void addToDictionary(String word, String translation)
throws IllegalArgumentException {
if (dict.containsKey(word)) {
throw new IllegalArgumentException(word + " already exists.");
}
dict.put(word, translation);
}

public synchronized Set getAllWords() {
return dict.keySet();
}
}

Here were the questions asked.

1. This class has a serious flaw. What is it and how can it be fixed?.
2. Improve performance of the class in concurrent environment using the Java Concurrency package.
Explain why performance is better.
3. Write a test which demonstrates performance improvement in a concurrent environment.

Thanks

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
peter__lawrey
Offline
Joined: 2005-11-01
Points: 0

> Can you give me a example to test the performance of the above code?

This is a lot trickier. You will need to get the other sections working before you start this.

There are not many examples of this as decent multi-threaded performance tests are rarely attempted by professional developers so I don't know what the person setting the question expected you to do here.
Do you have time to ask them?

peter__lawrey
Offline
Joined: 2005-11-01
Points: 0

> 1. This class has a serious flaw. What is it and how can it be fixed?.
getAllWords() returns a keySet() which is not thread safe.
- return a copy of the keySet
- or use ConcurrenHashMap instead of HashMap, then you can drop synchronized from translate and getAllWords
> 2. Improve performance of the class in concurrent environment using the Java Concurrency package.
Use ConcurrentHashMap
> Explain why performance is better.
I allows concurrent access and lower cost of access.
> 3. Write a test which demonstrates performance improvement in a concurrent environment.
I will leave that for you.

devuser007
Offline
Joined: 2009-01-17
Points: 0

Hi Peter,

Thanks a ton ..! That was indeed helpfull.
Would appreciate if you give me some insight on the Question 3 please?

Thanks

peter__lawrey
Offline
Joined: 2005-11-01
Points: 0

Unfortunately question 3 would take you about half a day if you haven't done that sort of thing before even with hints.
If I were to write some thing any marking it won't believe you wrote it. ;)

devuser007
Offline
Joined: 2009-01-17
Points: 0

Yeah thats true .! so please help me buddy..! I just need to clear this else m dead :(

devuser007
Offline
Joined: 2009-01-17
Points: 0

Can you give me a example to test the performance of the above code?

devuser007
Offline
Joined: 2009-01-17
Points: 0

In this code.
public class SlowDictionary {
private final Map dict = new HashMap();

public synchronized String translate(String word)
throws IllegalArgumentException {
if (!dict.containsKey(word)) {
throw new IllegalArgumentException(word + " not found.");
}
return dict.get(word);
}

public synchronized void addToDictionary(String word, String translation)
throws IllegalArgumentException {
if (dict.containsKey(word)) {
throw new IllegalArgumentException(word + " already exists.");
}
dict.put(word, translation);
}

public synchronized Set getAllWords() {
return dict.keySet();
}
}

instead of synchronizing the all methods can i use this

Map myMap = Collections.synchronizedMap (dict); so that i can have a single lock for the entire Map? does it make sense?

so this is how it will look

public class SlowDictionary {
private final Map map = new HashMap();

Map dict= Collections.synchronizedMap (map);

public String translate(String word)
throws IllegalArgumentException {
if (!dict.containsKey(word)) {
throw new IllegalArgumentException(word + " not found.");
}
return dict.get(word);
}

public void addToDictionary(String word, String translation)
throws IllegalArgumentException {
if (dict.containsKey(word)) {
throw new IllegalArgumentException(word + " already exists.");
}
dict.put(word, translation);
}

public Set getAllWords() {
return dict.keySet();
}
}
is it right?

peter__lawrey
Offline
Joined: 2005-11-01
Points: 0

> instead of synchronizing the all methods can i use this
> Map myMap = Collections.synchronizedMap (dict); so
> that i can have a single lock for the entire Map?
You aready have a single lock.
> does it make sense?
No. This approach is no better than what is there and is actually less safe.
> so this is how it will look
>
> public class SlowDictionary {
> private final Map map = new
> w HashMap();
>
> Map dict=
> = Collections.synchronizedMap (map);
>
> public String translate(String word)
> throws IllegalArgumentException {
> if (!dict.containsKey(word)) {
> throw new IllegalArgumentException(word + " not
> not found.");
> }
Note: this will release the lock between containsKey and get which is not desirable.
> return dict.get(word);
> }
>
> public void addToDictionary(String word, String
> g translation)
> throws IllegalArgumentException {
> if (dict.containsKey(word)) {
> throw new IllegalArgumentException(word + "
> + " already exists.");
> }
Note: this will release the lock between containsKey and put which is can allow a number of threads to get false from containsKey at the same time.
> dict.put(word, translation);
> }
>
> public Set getAllWords() {
> return dict.keySet();
This has the same problem I mentioned in the first reply.
> }
> }
> is it right?
At least you are trying now. How about trying the approach I suggested using ConcurrentHashMap ?

ConcurrentMap map = new ConcurrentHashMap()

HINT: Have a look at the methods which are defined in ConcurrentMap