powered by simpleCommunicator - 2.0.61     © 2026 Programmizd 02
Целевая тема:
Создать новую тему:
Автор:
Закрыть
Цитировать
Форумы / Java [игнор отключен] [закрыт для гостей] / codereview самописный потокобезопасный stack
25 сообщений из 120, страница 2 из 5
codereview самописный потокобезопасный stack
    #39399703
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
хотя это всё равно не позволит несколько читателей сразу... надо ещё подумать
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399704
no56892
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
авторчто конкретно не так с конструктором? какие проблемы могут быть?

мне кажется synchronized даёт все необходимые гарантии
А где у вас hb между вызовом конструктора и вызовом методов pop/push...? В предыдущей теме столько обсуждали...
Еще добавлю про Ваш код - обычно размер стека динамический, т.е. заранее не задается, в таком случае нафига мьютекс на запись? Ок, мы оставляем синхронайзд только для notify, но тут проблема - у нас то, где хранятся элементы совсем не конкаррент. Поэтому я бы сделал так:
Вот это lock-free стэк:
Код: java
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
16.
17.
18.
19.
20.
21.
22.
23.
24.
25.
26.
27.
28.
29.
30.
31.
32.
33.
34.
35.
36.
37.
38.
39.
class ConcurrentStack<E> {
	
	private final AtomicReference<Entry<E>> pointer = new AtomicReference<>();
	
	public void push(E element) {
		boolean finish = false;
		do {
			Entry<E> currentTop = pointer.get();
			Entry<E> newTop = new Entry<>(element, currentTop);
			finish = pointer.compareAndSet(currentTop, newTop);
		} while(!finish);
	}
	
	public E pop() {
		E ret = null;
		boolean finish = false;
		do {
			Entry<E> currentTop = pointer.get();
			Entry<E> newTop = null;
			if(currentTop != null) {
				ret = currentTop.element;	
				newTop = currentTop.prev;
			}
			finish = pointer.compareAndSet(currentTop, newTop);
		} while(!finish);
		return ret;
	}
	
	private static class Entry<E> {
		final E element; 
		final Entry<E> prev; 
		//здесь файналы не обязательны строго говоря, но так best practice (у нас hb //создает запись/чтение pointer

		Entry(E element, Entry<E> prev) {
			this.element = element;
			this.prev = prev;
		}
	}
}


А вот так - тот BlockingStack, который Вам и нужен:
Код: java
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
class BlockingStack<E> {
	
	final private ConcurrentStack<E> concurrentStack = new ConcurrentStack<E>();
	final private Semaphore blockStuff = new Semaphore(0);
	
	public void push(E element) {
		concurrentStack.push(element);
		blockStuff.release();
	}
	
	public E pop() throws InterruptedException {
		blockStuff.acquire();
		return concurrentStack.pop();
	}
}


questioner, Вам вот задание (ну Вы же сами просили) - написать метод size() для ConcurrentStack.
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399706
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
Чего-то я тупанул, всё оба действия ведь модифицируют коллекцию, соответственно 2 потока одновременно не могут производить операции

Код: java
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
16.
17.
18.
19.
20.
21.
22.
23.
24.
25.
 volatile AtomicBoolean locked = new AtomicBoolean(false);

    @Override
    public boolean offer(T t) {
        if (locked.compareAndSet(false, true)) {

            if (list.size() >= maxSize) {
                return false;
            }
            boolean addResult = list.add(t);
            locked.set(false);
            return addResult;
        }
        return false;
    }

    @Override
    public T take() {
        if (locked.compareAndSet(false, true)) {
            T t = list.get(list.size() - 1);
            locked.set(false);
            return t;
        }
        return null;
    }
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399712
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
вот такой вариант наверное будет работать, но медленно:

Код: java
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
16.
17.
18.
19.
20.
21.
22.
23.
24.
25.
26.
27.
28.
29.
30.
31.
32.
33.
34.
35.
36.
37.
38.
39.
40.
41.
42.
43.
44.
45.
46.
47.
48.
49.
50.
51.
52.
53.
54.
55.
56.
57.
58.
59.
60.
61.
62.
63.
64.
65.
66.
public interface ConcurrentStack<T> {
    //blocking
    void put(T t) throws InterruptedException;

    //blocking
    T poll() throws InterruptedException;

    //non-blocking put
    boolean offer(T t);

    //non-blocking get
    T take();
}

class ConcurrentStackImpl<T> implements ConcurrentStack<T> {
    private volatile AtomicBoolean locked = new AtomicBoolean(false);
    private List<T> internalList = new ArrayList<T>();
    private final int maxSize;

    public ConcurrentStackImpl(int maxSize) {
        if (maxSize <= 0) {
            throw new IllegalArgumentException();
        }
        this.maxSize = maxSize;
    }

    @Override //blocking
    public void put(T t) throws InterruptedException {
        while (!locked.compareAndSet(false, true)) ;//busy waiting
        internalList.add(t);
        locked.set(false);
    }

    @Override // non-blocking
    public T poll() throws InterruptedException {
        while (!locked.compareAndSet(false, true)) ;//busy waiting
        T value = internalList.get(internalList.size() - 1);
        locked.set(false);
        return value;
    }

    @Override
    public boolean offer(T t) {
        if (locked.compareAndSet(false, true)) {

            if (internalList.size() >= maxSize) {
                return false;
            }
            boolean addResult = internalList.add(t);
            locked.set(false);
            return addResult;
        }
        return false;
    }

    @Override
    public T take() {
        if (locked.compareAndSet(false, true)) {
            T t = internalList.get(internalList.size() - 1);
            locked.set(false);
            return t;
        }
        return null;
    }

}
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399716
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
no56892
Еще добавлю про Ваш код - обычно размер стека динамический, т.е. заранее не задается, в таком случае нафига мьютекс на запись? Ок, мы оставляем синхронайзд только для notify, но тут проблема - у нас то, где хранятся элементы совсем не конкаррент.


Не понял причинно-следственной связи. мы оставляем synchronized дабы исключить параллельное изменение коллекции потоками.
no56892 Поэтому я бы сделал так:
Вот это lock-free стэк:
Код: java
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
16.
17.
18.
19.
20.
21.
22.
23.
24.
25.
26.
27.
28.
29.
30.
31.
32.
33.
34.
35.
36.
37.
38.
39.
class ConcurrentStack<E> {
	
	private final AtomicReference<Entry<E>> pointer = new AtomicReference<>();
	
	public void push(E element) {
		boolean finish = false;
		do {
			Entry<E> currentTop = pointer.get();
			Entry<E> newTop = new Entry<>(element, currentTop);
			finish = pointer.compareAndSet(currentTop, newTop);
		} while(!finish);
	}
	
	public E pop() {
		E ret = null;
		boolean finish = false;
		do {
			Entry<E> currentTop = pointer.get();
			Entry<E> newTop = null;
			if(currentTop != null) {
				ret = currentTop.element;	
				newTop = currentTop.prev;
			}
			finish = pointer.compareAndSet(currentTop, newTop);
		} while(!finish);
		return ret;
	}
	
	private static class Entry<E> {
		final E element; 
		final Entry<E> prev; 
		//здесь файналы не обязательны строго говоря, но так best practice (у нас hb //создает запись/чтение pointer

		Entry(E element, Entry<E> prev) {
			this.element = element;
			this.prev = prev;
		}
	}
}




Вроде да, должно работать, но по-моему это то же самое, что я написал(спору нет, после Вас).

А вот так - тот BlockingStack, который Вам и нужен:
no56892
Код: java
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
class BlockingStack<E> {
	
	final private ConcurrentStack<E> concurrentStack = new ConcurrentStack<E>();
	final private Semaphore blockStuff = new Semaphore(0);
	
	public void push(E element) {
		concurrentStack.push(element);
		blockStuff.release();
	}
	
	public E pop() throws InterruptedException {
		blockStuff.acquire();
		return concurrentStack.pop();
	}
}




я не понял идею. явно где-то не хватает semaphore.take()

авторquestioner, Вам вот задание (ну Вы же сами просили) - написать метод size() для ConcurrentStack.[/quote]

Судя по всему надо, чтобы никто не работал в этот момент. Сейчас придумаю
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399720
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
ну и кстати концептуальный вопрос ещё:

конструкция вида:
Код: java
1.
2.
3.
4.
do{
  oldState = atom.get();//1
 //some sork
}while(atom.compareAndSet(oldState, newState))

//2

Как я понимаю мы пытаемся симулировать ситуацию, что между //1 и //2 никаких действий не произошло.

Но ведь за это время мог элемент добавиться и удалиться, а в итоге состояние то же с точки зрения нашей проверки.
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399721
забыл ник
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
questioner
Но ведь за это время мог элемент добавиться и удалиться, а в итоге состояние то же с точки зрения нашей проверки.

Гугли по ключевым словам проблема ABA
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399723
забыл ник
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
забыл никquestionerНо ведь за это время мог элемент добавиться и удалиться, а в итоге состояние то же с точки зрения нашей проверки.

Гугли по ключевым словам проблема ABA

В языках с управляемой памятью не критично, в данном случае все будет ок
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399729
no56892
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
авторНе понял причинно-следственной связи. мы оставляем synchronized дабы исключить параллельное изменение коллекции потоками.
Написал немного не то, что хотел сказать

авторВроде да, должно работать, но по-моему это то же самое, что я написал(спору нет, после Вас).
Это совершенно разные вещи, Вы написали опять с локами, только в отличие от первого варианта с synchronized (который, кстати хотя бы работает), здесь у Вас формально CAS, но только формально, в Вашей реализации это те же локи. Написать над методом non blocking и вставиеь busy-wait - это мощно.

авторя не понял идею. явно где-то не хватает semaphore.take()
В Java SE API))

авторСудя по всему надо, чтобы никто не работал в этот момент. Сейчас придумаю
Опять локи?

По коду:
Код: java
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
@Override
    public boolean offer(T t) {
        if (locked.compareAndSet(false, true)) {

            if (internalList.size() >= maxSize) { // (A)
                return false; 
            }
            boolean addResult = internalList.add(t);
            locked.set(false); // (B) 
            return addResult;
        }
        return false;
    }


Если срабатывает условие (A) весь стэк навесгда в busy-wait...Ну и немного не понятное у Вас Апи - через offer мы не можем больше max положить, а через put - можем?

Код: java
1.
2.
3.
4.
5.
6.
7.
8.
9.
@Override
    public T take() {
        if (locked.compareAndSet(false, true)) { // (A)
            T t = internalList.get(internalList.size() - 1);
            locked.set(false);
            return t;
        }
        return null;
    }


(A) - допустим у нас есть 1+ элементов в стэке уже и один поток уже получил лок и кладет его туда, тоесть получается вернет null? Хотя там есть элементы...

Нафига AtomicBoolean volatile? Конструктор не safe с точки зрения JLS, хотя на практике спасет хотя бы одно final поле, но я бы так не писал.

Ну и все эти
Код: java
1.
2.
3.
 while (!locked.compareAndSet(false, true)) ;
...
locked.set(false);


ничем не отличаются от lock.lock()/unlock() кроме того, что забываете unlock делать.

авторКак я понимаю мы пытаемся симулировать ситуацию, что между //1 и //2 никаких действий не произошло.
Если произошло - повторяем цикл заново. "!" забыли.
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399731
no56892
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
no56892ничем не отличаются от lock.lock()/unlock() кроме того, что забываете unlock делать
Ну и еще невозможностью "интерраптнуть"
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399738
no56892
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
no56892no56892ничем не отличаются от lock.lock()/unlock() кроме того, что забываете unlock делать
Ну и еще невозможностью "интерраптнуть"
Нет, такое использование еще даже хуже - процессор жрет.
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399806
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
забыл никзабыл никпропущено...


Гугли по ключевым словам проблема ABA

В языках с управляемой памятью не критично, в данном случае все будет ок

Да, я гуглил и видел фразу, что в языках с gc ее тяжело воспроизвести. Почему в данном случае все ок будет? Проверка ведь будет успешной.

Смотрите, еще раз, сценарий такой:

1.Первый поток запомнил head
2. Далее другие потоки успели добавить и удалить элемент из стекла
3. Первый поток проверяет, что old == pointer и цикл кончится



Почему мы здесь эту проблему не получим?
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399813
Сергей Арсеньев
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
questionerВдруг будет аргументированная критика.
Ну,
1. Если назвать NonBlocking и потом все на synchronized методах это сильно.
2. Долго выяснять зачем ставить final на поля типа list и maxsize и не сделать этого.
3. Размер списка не уменьшается, мелочь конечно, но тогда почему бы не
Код: java
1.
list = new ArrayList<T>(this.maxSize = maxSize);


4. remove само по себе возвращает объект, повторный поиск лишний.
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399816
Сергей Арсеньев
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
questionerхотя это всё равно не позволит несколько читателей сразу... надо ещё подумать
А нафиг стеку параллельные читатели-писатели?
Овчинка выделки не стоит.

P.S. И мне кажется что массива с указателем текущего положения будет вполне достаточно, ArrayList практически не нужен.
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399822
Сергей Арсеньев
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
А главное, кого будум ждать, если тот, кто должен сделать следующий pull пытается сделать push, а стек переполнен.
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399825
Сергей Арсеньев
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
Кстати про remove.

Если один и тот же объект у тебя лежит два раза в list.
Что будет по remove(t)?
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399833
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
Сергей АрсеньевquestionerВдруг будет аргументированная критика.
Ну,
1. Если назвать NonBlocking и потом все на synchronized методах это сильно.

это уже обсудили, Вы не дочитали
Сергей Арсеньев 2. Долго выяснять зачем ставить final на поля типа list и maxsize и не сделать этого.

Похоже и там и там правда нужен. Была мысль, что раз меняем в sync секции, то и не надо, но начальная инициализация может ускользнуть

Сергей Арсеньев 3. Размер списка не уменьшается, мелочь конечно, но тогда почему бы не
Код: java
1.
list = new ArrayList<T>(this.maxSize = maxSize);



такая задача не ставилась

Сергей Арсеньев 4. remove само по себе возвращает объект, повторный поиск лишний.
согласен, но не этот аспект был интересен, поэтому не заморчивался
Сергей АрсеньевЕсли один и тот же объект у тебя лежит два раза в list.
Что будет по remove(t)?

удалится первый, а нужен последний, согласен, но, эти кейсы не столь интересны для меня в этой задаче
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399864
Сергей Арсеньев
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
questionerудалится первый, а нужен последний, согласен, но, эти кейсы не столь интересны для меня в этой задаче
Для начала надо понять задачу, а потом ее параллелить. Это основной принцип.
Если Вы не уделяете внимания тому, что делаете что вставляем, что удаляем, то браться за параллельное выполнение этих действий - только плодить множество ошибок.

Да и сам выбор задачи не удачен. Профита от неблокирования тут никакого. Тут еще есть смысл в подборе правильной блокировки, поиске зацикливаний и т.п., но не более того.

В смысле неблокирующих операций лучше посмотреть на вариант перевода средств между счетами типа списать у Иванова 10 записать Петрову (желательно, чтоб денег хватало, не было бы взаимоблокировок и чтоб Сидоров не обогатился на этом).
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399911
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
забыл никзабыл никпропущено...


Гугли по ключевым словам проблема ABA

В языках с управляемой памятью не критично, в данном случае все будет ок

а тут пишут, что вполне может

http://www.ibm.com/developerworks/library/j-jtp11234/
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399920
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
no56892,

no56892Это совершенно разные вещи, Вы написали опять с локами, только в отличие от первого варианта с synchronized (который, кстати хотя бы работает), здесь у Вас формально CAS, но только формально, в Вашей реализации это те же локи. Написать над методом non blocking и вставиеь busy-wait - это мощно.


а вот я правда не очень понимаю чем мой цикл от Вашего отличается. Насчёт дырок в логике, да, всё верно.

скопирую рядом:
Код: java
1.
2.
3.
4.
5.
6.
7.
8.
public void push(E element) {
		boolean finish = false;
		do {
			Entry<E> currentTop = pointer.get();
			Entry<E> newTop = new Entry<>(element, currentTop);
			finish = pointer.compareAndSet(currentTop, newTop);
		} while(!finish);
	}





Код: java
1.
2.
3.
4.
5.
6.
     @Override //blocking
    public void put(T t) throws InterruptedException {
        while (!locked.compareAndSet(false, true)) ;//busy waiting
        internalList.add(t);  //да, здесь нужна проверка на размер предварительно
        locked.set(false);
    }



по-моему у Вас наоборот плохо в цикле, что каждый раз новый объект создаётся. у меня вот один раз я захватил всё добро и только тогда делаю объек, хотя это время лока удлиняет. наверное один раз заранее лучше сделать
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399921
забыл ник
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
questionerзабыл никпропущено...


В языках с управляемой памятью не критично, в данном случае все будет ок

а тут пишут, что вполне может

http://www.ibm.com/developerworks/library/j-jtp11234/

Опять видимо в ваших фантазиях.
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399924
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
забыл никquestionerпропущено...


а тут пишут, что вполне может

http://www.ibm.com/developerworks/library/j-jtp11234/

Опять видимо в ваших фантазиях.

аргументированно
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399931
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
questionerno56892,

no56892Это совершенно разные вещи, Вы написали опять с локами, только в отличие от первого варианта с synchronized (который, кстати хотя бы работает), здесь у Вас формально CAS, но только формально, в Вашей реализации это те же локи. Написать над методом non blocking и вставиеь busy-wait - это мощно.


а вот я правда не очень понимаю чем мой цикл от Вашего отличается. Насчёт дырок в логике, да, всё верно.

скопирую рядом:
Код: java
1.
2.
3.
4.
5.
6.
7.
8.
public void push(E element) {
		boolean finish = false;
		do {
			Entry<E> currentTop = pointer.get();
			Entry<E> newTop = new Entry<>(element, currentTop);
			finish = pointer.compareAndSet(currentTop, newTop);
		} while(!finish);
	}





Код: java
1.
2.
3.
4.
5.
6.
     @Override //blocking
    public void put(T t) throws InterruptedException {
        while (!locked.compareAndSet(false, true)) ;//busy waiting
        internalList.add(t);  //да, здесь нужна проверка на размер предварительно
        locked.set(false);
    }



по-моему у Вас наоборот плохо в цикле, что каждый раз новый объект создаётся. у меня вот один раз я захватил всё добро и только тогда делаю объек, хотя это время лока удлиняет. наверное один раз заранее лучше сделать

Мысли спутались пока писал)) у меня ж нет объекта никакого нового, мне его передают.

а у Вас было бы оптимальнее создать объект до цикла, в начале цикла взять струю голову, у созданного объекта настроить ссылку на старый и вызвать CAS
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399937
забыл ник
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Участник
questionerзабыл никпропущено...


Опять видимо в ваших фантазиях.

аргументированно

Мусор на входе = мусор на выходе
...
Рейтинг: 0 / 0
codereview самописный потокобезопасный stack
    #39399938
questioner
Скрыть профиль Поместить в игнор-лист Сообщения автора в теме
Гость
забыл никquestionerпропущено...


аргументированно

Мусор на входе = мусор на выходе

я ссылку привел. вот цитата из нее:

http://www.ibm.com/developerworks/library/j-jtp11234/ The ABA problem
Because CAS basically asks "is the value of V still A" before changing V, it is possible for a CAS-based algorithm to be confused by the value changing from A to B and back to A between the time V was first read and the time the CAS on V is performed. In such a case, the CAS operation would succeed, but in some situations the result might not be what is desired. (Note that the counter and mutex examples from Listing 1 and Listing 2 are immune to this problem, but not all algorithms are.) This problem is called the ABA problem, and is generally dealt with by associating a tag, or version number, with each value to be CASed, and atomically updating both the value and the tag. The AtomicStampedReference class provides support for this approach.


Статья про атомики
...
Рейтинг: 0 / 0
25 сообщений из 120, страница 2 из 5
Форумы / Java [игнор отключен] [закрыт для гостей] / codereview самописный потокобезопасный stack
Найденые пользователи ...
Разблокировать пользователей ...
Читали форум (0):
Пользователи онлайн (0):
x
x
Закрыть


Просмотр
0 / 0
Close
Debug Console [Select Text]