JDK-8367717 기여기: atomic_copy64 정리하기

작년 10월에 JDK-8367717 이슈로 PR을 올렸는데, 논의가 멈춘 채로 1년 가까이 지났다. 최근 David Holmes가 범위를 정리해 줘서 다시 진행하게 됐다. 그런데 너무 오랜만에 열어 보니 무슨 이슈였는지부터 기억이 안 났다. 그래서 나부터 다시 이해할 겸 이슈 내용과 작업을 정리해 둔다.

이슈의 제목은 다음과 같다.

Cleanup atomic_copy64

이슈 내용

이슈는 Kim Barrett이 올렸다. 본문은 다음과 같다.

atomic_copy64 for {bsd,linux}-zero are defined in atomicAccess{bsd,linux}_zero.hpp and used in os{bsd,linux}zero.cpp. Other (non-zero) platform definitions of atomic_copy64 are defined in their respective os{os}_{arch}.cpp files, where the only uses are located. It would be better of the zero platfors were consistent with the others.

On the other hand, we have, for some time, required all platforms to support atomic load/store of 64bit values. As a result, all definitions of atomic_copy64 are essentially equivalent to AtomicAccess::store((int64_t*)dst, AtomicAccess::load((int64_t*)src))

Maybe we should just delete all definitions of atomic_copy64 and use that formulation instead.

Zero 플랫폼(bsd, linux)의 atomic_copy64는 atomicAccess{bsd,linux}_zero.hpp에 정의되어 있고 os{bsd,linux}zero.cpp에서 쓰인다. 다른(Zero가 아닌) 플랫폼의 atomic_copy64는 유일한 사용처인 각자의 os{os}_{arch}.cpp 파일에 정의되어 있다. Zero 플랫폼도 나머지와 일관되게 맞추는 편이 좋겠다.

한편, 꽤 오래전부터 모든 플랫폼은 64비트 값의 atomic load/store를 지원해야 한다. 그러니 atomic_copy64의 정의는 모두 사실상 AtomicAccess::store((int64_t*)dst, AtomicAccess::load((int64_t*)src))와 같다.

어쩌면 atomic_copy64 정의를 전부 지우고 이 식으로 대체하는 편이 나을지도 모른다.

이슈에는 David Holmes의 코멘트도 달려 있다.

We have always required all platforms to support 64-bit atomic load/store for use with Java volatile long/double, but the code to do it on 32-bit platforms varied widely. Even so I suspect we would have to been able to replace this with Atomic::load+Atomic::store a very long time ago. So I’d say we should get rid of atomic_copy64 altogether. (Though I hope we can avoid the casts!).

Java의 volatile long/double을 위해 모든 플랫폼은 항상 64비트 atomic load/store를 지원해야 했다. 다만 32비트 플랫폼에서 이를 구현하는 코드는 제각각이었다. 그래도 아주 오래전에 이미 Atomic::load + Atomic::store로 대체할 수 있었을 것 같다. 그러니 atomic_copy64는 아예 없애자. (캐스트는 피할 수 있으면 좋겠다!)

정리하면 선택지가 두 가지였다.

1. Zero의 atomic_copy64 정의를 다른 플랫폼처럼 os_*.cpp 쪽으로 옮겨 위치만 맞춘다.
2. atomic_copy64를 아예 지우고 AtomicAccess::load/store로 바꾼다.

배경지식

atomic_copy64는 어디에 쓰이나

atomic_copy64는 64비트 값 하나를 atomic하게 읽어서 atomic하게 쓰는 헬퍼다. 쓰이는 곳은 딱 하나, 각 플랫폼의 _Copy_conjoint_jlongs_atomic()이다.

이 함수는 Copy::conjoint_jlongs_atomic()의 플랫폼별 구현이다. src/hotspot/share/utilities/copy.hpp의 주석을 보면 Copy 클래스의 복사 함수는 몇 가지 속성으로 이름이 붙는다.

  // Block copy methods have four attributes.  We don't define all possibilities.
  //   alignment: aligned to BytesPerLong
  //   arrayof:   arraycopy operation with both operands aligned on the same
  //              boundary as the first element of an array of the copy unit.
  //              This is currently a HeapWord boundary on all platforms, except
  //              for long and double arrays, which are aligned on an 8-byte
  //              boundary on all platforms.
  //              arraycopy operations are implicitly atomic on each array element.
  //   overlap:   disjoint or conjoint.
  //   copy unit: bytes or words (i.e., HeapWords) or oops (i.e., pointers).
  //   atomicity: atomic or non-atomic on the copy unit.

conjoint_jlongs_atomic이라는 이름은 “겹칠 수 있는(conjoint) 영역에서, jlong 단위로, 각 원소를 atomic하게” 복사한다는 뜻이다. long[] 배열 복사(accessBackend.cpp) 같은 곳에서 쓰인다.

“각 원소를 atomic하게”라는 조건이 중요하다. 다른 스레드가 복사 중인 배열을 동시에 읽더라도 long 하나가 상위 32비트는 옛날 값, 하위 32비트는 새 값처럼 찢어진(tearing) 상태로 보이면 안 된다는 것이다.

구현은 대략 이렇다. (os_linux_aarch64.cpp)

  void _Copy_conjoint_jlongs_atomic(const jlong* from, jlong* to, size_t count) {
    if (from > to) {
      const jlong *end = from + count;
      while (from < end)
        atomic_copy64(from++, to++);
    }
    else if (from < to) {
      const jlong *end = from;
      from += count - 1;
      to   += count - 1;
      while (from >= end)
        atomic_copy64(from--, to--);
    }
  }

영역이 겹칠 수 있으므로 from과 to의 위치에 따라 앞에서부터 또는 뒤에서부터 복사한다. 원소 하나하나는 atomic_copy64로 옮긴다.

플랫폼마다 달랐던 정의

atomic_copy64는 이 함수를 구현하는 os_cpu 디렉터리에 각각 정의되어 있었다.

aarch64와 riscv는 64비트 플랫폼이라 정렬된 8바이트 접근을 한 번에 처리할 수 있다. 그래서 그냥 대입문으로 썼다.

Zero는 사정이 다르다. Zero는 어셈블리 없이 C++만으로 돌아가는 인터프리터 전용 포트라서 32비트 ARM, PPC 같은 플랫폼에서도 빌드된다. 32비트에서 jlong 대입을 그냥 쓰면 컴파일러가 32비트 load/store 두 번으로 쪼갤 수 있으므로 별도 처리가 필요하다.

AtomicAccess

AtomicAccess(src/hotspot/share/runtime/atomicAccess.hpp)는 HotSpot에서 atomic 연산을 담당하는 클래스다. 공통 인터페이스 아래에 플랫폼별 구현(PlatformLoad, PlatformStore 등)을 특수화로 끼워 넣는 구조다.

HotSpot은 모든 플랫폼에서 64비트 atomic load/store를 요구한다. AtomicAccess::load/store를 쓰면 각 플랫폼은 자기에게 맞는 방법으로 tearing 없이 처리해 준다. 더 이상 atomic_copy64 같은 헬퍼를 os_cpu 디렉터리마다 따로 들고 있을 필요가 없다.

이전에는 Atomic의 load/store가 jbyte, jint, jlong처럼 타입별 오버로드로 나뉘어 있었고, 플랫폼마다 이를 하나씩 구현했다. 지금은 템플릿 기반의 통일된 인터페이스가 있으니 이걸로 대체하면 된다.

진행 과정

첫 번째 시도: 위치만 맞추기

처음에는 이슈 본문의 첫 문단을 따라 Zero의 atomic_copy64를 atomicAccess_*_zero.hpp에서 os_*_zero.cpp로 옮겼다. 다른 플랫폼과 마찬가지로 유일한 사용처 옆에 static inline으로 두는 방식이다.

이슈 본문이 “On the other hand”로 두 가지 방향을 모두 제시하고 있어서 이번 티켓에서는 정의 위치만 맞추고 AtomicAccess로 바꾸는 건 다음 티켓에서 따로 하자는 뜻으로 이해했다.

리뷰: 아예 지우자

하지만 Kim Barrett이 바로 Request changes를 남겼다.

The issue suggests it might be better to replace calls to atomic_copy64 with AtomicAccess::store(dst, AtomicAccess::load(src)) (using the new AtomicAccess nomenclature), and then delete all definitions of atomic_copy64. And there’s a comment agreeing with that suggestion. I think that’s the way to go, rather than what this PR proposes.

이슈에서는 atomic_copy64 호출을 (새 AtomicAccess 명명을 써서) AtomicAccess::store(dst, AtomicAccess::load(src))로 바꾸고, atomic_copy64 정의를 모두 지우는 편이 나을 수 있다고 제안하고 있다. 그 제안에 동의하는 코멘트도 있다. 이 PR이 제안하는 방식보다 그쪽이 맞다고 생각한다.

이슈를 올린 사람이 직접 두 번째 방향이 맞다고 정리해 준 것이다. 그래서 PR을 두 번째 방향으로 다시 작성했다.

범위가 넓어지다

그런데 수정한 PR에 Justin King(jcking)이 코멘트를 달면서 이야기가 커졌다.

This partially fixes https://bugs.openjdk.org/browse/JDK-8369207 as well, but this only does it for jlong. I can wait for this to go in, and then update jshort and jint to use AtomicAccess as well or we can do it all in this PR.

이 PR은 JDK-8369207도 일부 고친다. 하지만 jlong만 고친다. 이게 들어간 다음에 내가 jshort, jint도 AtomicAccess를 쓰도록 바꿔도 되고, 이 PR에서 한꺼번에 해도 된다.

JDK-8369207은 “Copy_conjoint{jshorts,jints,jlongs}_atomic on Linux/BSD AArch64 do not prevent tearing”이라는 이슈다. aarch64의 atomic_copy64는 그냥 *dst = *src 대입문이라, 하드웨어는 8바이트 접근을 한 번에 처리하더라도 C++ 컴파일러 입장에서는 이 접근을 atomic으로 다뤄야 한다는 보장이 없다. 같은 파일의 jshort, jint 버전도 마찬가지로 평범한 대입문이다. 이 PR은 jlong 쪽만 AtomicAccess로 바꾸니 그 이슈의 일부만 해결하는 셈이다.

Kim Barrett은 여기서 더 나아갔다.

Note that your’s currently doesn’t perform atomic accesses on the “from” value. I think that’s wrong, since the documentation for these functions doesn’t indicate atomicity only for the store. The comment in copy.hpp says “atomicity: atomic or non-atomic on the copy unit.” I take that to mean both source and destination.

Your PR is also not addressing other platforms with similar issues, such as in copy_ppc.hpp.

Also, this all begs the question of why we have four different copies of these functions - bsd_aarch64, bsd_zero, linux_aarch64, and linux_zero. All of these should be identical.

Basically, I think the Copy class is kind of a mess, and that discourages its use. Probably this needs an overall plan of attack (broken up into reviewable chunks, but with an overarching goal), rather than piecemeal fixups.

(jcking의 PR은) 현재 “from” 값에 대해서는 atomic 접근을 하지 않는다. 이 함수들의 문서는 store에만 atomic하다고 하지 않으니 이건 잘못된 것 같다. copy.hpp의 주석은 “atomicity: atomic or non-atomic on the copy unit”이라고 하는데, 나는 이걸 source와 destination 모두라고 이해한다.

copy_ppc.hpp처럼 비슷한 문제가 있는 다른 플랫폼도 다루지 않는다.

그리고 애초에 bsd_aarch64, bsd_zero, linux_aarch64, linux_zero에 이 함수들의 사본이 네 벌이나 있는 이유가 뭔지 의문이다. 전부 같아야 한다.

기본적으로 Copy 클래스는 좀 엉망이고, 그래서 사람들이 잘 안 쓰게 된다. 땜질식 수정보다는 (리뷰 가능한 단위로 나누되 큰 목표를 가진) 전체적인 계획이 필요할 것 같다.

이어서 jcking이 AtomicAccess처럼 기본 구현을 두고 플랫폼별로 특수화하는 구조를 제안했다. 하지만 거기서 논의가 더 진행되지 않았다.

내 PR은 처음에 atomic_copy64 하나를 정리하는 작은 작업이었다. 그런데 이야기가 Copy 클래스 전체를 어떻게 재설계할지로 넘어가 버려서 어디까지 손대야 할지 판단하기가 어려웠다. 결국 12월에 논의가 정리되지 않은 것 같다는 코멘트를 남기고 PR을 draft로 돌렸다.

다시 좁혀진 범위

그렇게 1년 가까이 멈춰 있던 PR에 올해 9월 David Holmes가 코멘트를 남겼다.

Can we re-open this as-is and keep the scope simply to replacing atomic_copy64 with the AtomicAccess load and store operations.

지금 상태 그대로 다시 열고, 범위는 단순히 atomic_copy64를 AtomicAccess load/store 연산으로 바꾸는 것으로 한정하자.

Copy 클래스 전체 정리나 jshort/jint의 tearing 문제는 별도 작업으로 두고, 이 PR은 이슈 제목 그대로 atomic_copy64 정리만 하자는 것이다. 나는 이 코멘트를 못 보고 있다가, 한 달쯤 지나 David Holmes에게서 개인 메일이 오고 나서야 확인했다. 늦게 답한 것을 사과하고 최신 master를 머지해서 PR을 다시 열었다.

수정 내용

1. 정의 삭제

각각 존재하던 atomic_copy64 정의 다섯 개를 모두 지웠다.

• src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp
• src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp
• src/hotspot/os_cpu/linux_riscv/os_linux_riscv.cpp
• src/hotspot/os_cpu/linux_zero/atomicAccess_linux_zero.hpp
• src/hotspot/os_cpu/bsd_zero/atomicAccess_bsd_zero.hpp

2. 호출부 교체

_Copy_conjoint_jlongs_atomic()의 호출부를 AtomicAccess::store(dst, AtomicAccess::load(src))로 바꿨다.

     if (from > to) {
       const jlong *end = from + count;
       while (from < end)
-        atomic_copy64(from++, to++);
+        AtomicAccess::store(to++, AtomicAccess::load(from++));
     }
     else if (from < to) {
       const jlong *end = from;
       from += count - 1;
       to   += count - 1;
       while (from >= end)
-        atomic_copy64(from--, to--);
+        AtomicAccess::store(to--, AtomicAccess::load(from--));
     }

from, to가 이미 const jlong*, jlong*라서 이슈 본문의 (int64_t*) 캐스트는 필요 없었다. David Holmes가 “캐스트는 피할 수 있으면 좋겠다”고 한 부분도 자연스럽게 해결됐다.

Java 개발자에게는 어떤 영향이 있을까

작업을 마치고 나니, 그래서 Java 개발자는 이 함수를 어디서 만나게 되는지 궁금해졌다. 코드를 따라가 보면서 정리해 봤다.

System.arraycopy에서 내려오는 경로

개발자가 이 함수의 존재나 이번 변경을 의식할 일은 거의 없다. 그래도 내부적으로는 꽤 자주 지나가는 함수다.

가장 대표적인 경로는 인터프리터 모드에서 원시 타입 배열에 System.arraycopy를 호출할 때다.

long[] src = {1L, 2L, 3L, 4L};
long[] dst = new long[4];
System.arraycopy(src, 0, dst, 0, 4); // Arrays.copyOf(long[]) 등도 내부적으로 이걸 부른다

System.arraycopy 는 String, StringBuilder, ArrayList, Arrays.copyOf 같은 코드 내부에서도 사용된다.

마무리

코드만 보면 정의 다섯 개를 지우고 호출부 열 곳을 한 줄씩 바꾼 작업이다. 그런데 리뷰 과정에서 범위가 Copy 클래스 전체로 커졌다가, 1년 가까이 지나서 다시 원래 범위로 돌아왔다.

리뷰어끼리 더 큰 그림을 이야기하기 시작하자, 이 PR에서 무엇을 더 해야 하는지 알 수 없게 됐다. 결국 범위 정리는 David Holmes가 해 주었다. 다음에 비슷한 상황이 오면 “이 PR의 범위는 여기까지로 하고 나머지는 별도로 진행해도 될까요?”라고 먼저 이야기 해봐야겠다.

참고

• JDK-8367717: Cleanup atomic_copy64
• openjdk/jdk#27642
• JDK-8369207: Copy_conjoint{jshorts,jints,jlongs}_atomic on Linux/BSD AArch64 do not prevent tearing
• JDK-8319883: Zero: Use atomic built-ins for 64-bit accesses