-
Notifications
You must be signed in to change notification settings - Fork 3
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
Buffer overhaul #7
Conversation
|
e71795f
to
07ebb95
Compare
d643080
to
c59246a
Compare
c59246a
to
e1a5c80
Compare
e1a5c80
to
e4f1d8b
Compare
이 pr은 Result<T, B> 를 Result<(T, B), Error(B)> 로 변경된거라고 보면 되나요? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buffer에 대해 아주 아래쪽 레이어의 구조체로 쓰게 될 것 같아 최소한의 추상화만 가지면 좋겠다는 생각이 들어 두 가지를 논의해보고싶은데요,
-
user_data필드로 Vec 을 처리하게 wrapping 하는건 안되는지?
현재 내부에 BufferSource, total_size_in_bytes 등 Vec을 따로 처리해주기 위한 필드들이 혼재되어있어서 복잡함을 야기하는 것 같습니다. 추상화 레이어를 쌓아서 이 둘을 제거하고 Vec을 버퍼와 서로 변환되게 잘 작성할 수 있어야 하는게 아닐까 싶습니다. (그게 안된다면 애초에 BufferState 가 다른 구조체를 위한 용도로 쓰기 어렵다는 반증이기도 한 것 같습니다.) -
vector i/o가 기본인 구조체로 Buffer를 구성하신 것 같은데 마찬가지로 이게 또 복잡도를 야기하고 추상화 레이어를 서로 침범하는 것 같습니다. vector i/o와 buffer를 개념상 분리해서 다룰 수 있게 구현할 순 없을까요? 예를들어 Vec를 받는 경우 vector i/o를 하도록이요.
코드를 좀 더 보니 의도하신건 r,w,vectored r,w 에 대해 모두 동일한 내부 데이터타입을 가지게 해서 uring link등을 구성할 때에 타입 복잡도를 낮게 구현하려는 목적이 있으신 것 같네요. 추상화 레이어를 구성하고 의도에 맞게 짜려면 Buffer를 가장 하부 구조체로 두고 (Buffer에서는 Vec에 대한 해석을 하지 않도록 복잡도를 낮출고), Vec, Vec<Vec>을 서로 변환하는 레이어가 필요할 것 같기도 합니다.
1번 제안은 제가 잘 이해를 못했는데, Vec wrapping하는게 무슨 뜻인지 잘 모르겠네요. 지금 구조랑 의도만 먼저 설명드리면 일단 underlying buffer == Buffer source (e.g. Vec)를 모르게 디자인 한건 맞고요. 무조건 있어야 하는 공통 요소는 iovec을 만들기 위한 ptr, size 입니다. 나머지 data는 source 를 다시 변환하기 위한 field이고요. BufferState는 ptr, size만 갖고 있는걸 의도한것이라 vector랑은 무관하긴 합니다. 2번은 의도적으로 vector까지 포함하는 형태를 만든건데요, 말씀하신 link를 위함도 있고 성능과 api를 위함(기존 이슈에 기술된)이기도 합니다. 근데 사실 저도 써보면서 그 두개를 합친게 불편함이 없지는 않아서 조금 고민이긴 한데요. buffer 디자인이 미치는 영향이 크니 wg 회의때 조금 더 의견 나눠보면 좋을거 같아요. |
|
용우님이 fixed buf 추가해주시면서 위 요구사항도 같이 반영해주셨습니다. BufferSource에 underlying buffer specific한 data를 넣는 형태로 수정해주셨어요. |
BufferSource에 다른 타입의 버퍼가 들어가야 하면 tokio-uring을 패치해야 하나요? |
이건 추가적인 리팩토링(ileixe#3) 으로 BufferImpl만 구현하면 되도록 변경되었습니다. |
Refactoring Buffer using BufferImpl
BufResult
to bestd::result::Result
tokio-rs/tokio-uring#178Result<T, B>
to beResult<(T,B), Error<B>
tokio-rs/tokio-uring#295