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

Buffer overhaul #7

Merged
merged 13 commits into from
Nov 19, 2024
Merged

Buffer overhaul #7

merged 13 commits into from
Nov 19, 2024

Conversation

@ileixe ileixe requested a review from a team September 30, 2024 05:20
@ileixe
Copy link
Collaborator Author

ileixe commented Sep 30, 2024

fork에다가 custom change들을 쌓는게 좋을거 같아서 default branch를 변경했습니다. fixed buf는 Buffer 구현에 녹일 수 있을 거 같아서 일단 기존 custom 코드들은 빼두었습니다.

src/buf/mod.rs Outdated Show resolved Hide resolved
src/buf/fixed/pool.rs Show resolved Hide resolved
@kstreee-furiosa
Copy link

이 pr은 Result<T, B> 를 Result<(T, B), Error(B)> 로 변경된거라고 보면 되나요?

Copy link

@kstreee-furiosa kstreee-furiosa left a comment

Choose a reason for hiding this comment

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

Buffer에 대해 아주 아래쪽 레이어의 구조체로 쓰게 될 것 같아 최소한의 추상화만 가지면 좋겠다는 생각이 들어 두 가지를 논의해보고싶은데요,

  1. user_data필드로 Vec 을 처리하게 wrapping 하는건 안되는지?
    현재 내부에 BufferSource, total_size_in_bytes 등 Vec을 따로 처리해주기 위한 필드들이 혼재되어있어서 복잡함을 야기하는 것 같습니다. 추상화 레이어를 쌓아서 이 둘을 제거하고 Vec을 버퍼와 서로 변환되게 잘 작성할 수 있어야 하는게 아닐까 싶습니다. (그게 안된다면 애초에 BufferState 가 다른 구조체를 위한 용도로 쓰기 어렵다는 반증이기도 한 것 같습니다.)

  2. 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>을 서로 변환하는 레이어가 필요할 것 같기도 합니다.

@ileixe
Copy link
Collaborator Author

ileixe commented Oct 30, 2024

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 회의때 조금 더 의견 나눠보면 좋을거 같아요.

@kstreee-furiosa
Copy link

kstreee-furiosa commented Oct 30, 2024

나머지 data는 source 를 다시 변환하기 위한 field이고요. BufferState는 ptr, size만 갖고 있는걸 의도한것이라 vector랑은 무관하긴 합니다.

total_size_in_bytes 필드와 BufferSource struct가 Buffer내부에 포함되지 않게 구현할 수 있을까요? 저 필드와 스트럭트는 rust Vec를 처리해주기 위한 구체적인 요인들이라 이 두 구조체가 존재하는 한 아래가 성립하지 않는 것 같습니다.

일단 underlying buffer == Buffer source (e.g. Vec)를 모르게 디자인 한건 맞고요

@ileixe
Copy link
Collaborator Author

ileixe commented Nov 1, 2024

용우님이 fixed buf 추가해주시면서 위 요구사항도 같이 반영해주셨습니다. BufferSource에 underlying buffer specific한 data를 넣는 형태로 수정해주셨어요.

@kstreee-furiosa
Copy link

kstreee-furiosa commented Nov 6, 2024

BufferSource에 다른 타입의 버퍼가 들어가야 하면 tokio-uring을 패치해야 하나요?

@ileixe
Copy link
Collaborator Author

ileixe commented Nov 19, 2024

BufferSource에 다른 타입의 버퍼가 들어가야 하면 tokio-uring을 패치해야 하나요?

이건 추가적인 리팩토링(ileixe#3) 으로 BufferImpl만 구현하면 되도록 변경되었습니다.

@ileixe ileixe merged commit 838cf25 into furiosa-ai:fork Nov 19, 2024
@ileixe ileixe deleted the buffer-overhaul branch November 19, 2024 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants