[refactor]: TemporalFilter 라인 차트 데이터 처리 성능 최적화#841
Conversation
- Replace forEach with for-of loop for better performance - Simplify map operations using nullish coalescing operator - Use more efficient Array.from with mapping function - Remove unnecessary intermediate variables and helper functions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
ytaek
left a comment
There was a problem hiding this comment.
디테일한 부분에 대한 최적화는 매우 환영합니다!!
- ms 단위의 최적화 결과를 분석하셨는데, 분석 기법들은 너무나도 좋은 것 같습니다. 변동계수까지!! 👍👍👍👍👍
- 워밍업과 5회 분석, 상하위 10% 이상치 제거 등을 보고 감동까지 했습니다!!! 💯💯💯
조금 아쉬운게 있다면,
- 리뷰에 남긴 것처럼, 성능 vs 가독성/유지보수성 등에 대한 trade-off 분석/고민이 필요할 수 있을 것 같습니다. 함수로 뺀걸 풀어서 쓰는 일들은 대부분의 refactoring에서 얘기하는 걸 거꾸로 행하는 방식이기도 하구요.
- 각 변경 아이템들 하나하나에 대해서 성능 측정이 이루어졌으면 좋았을 것 같습니다. 위에서 얘기한 다른 품질 속성들에 대한 크게 고려를 하지 않아도 되는 무해(?)한 변경들 (ex. forEach -> for-of) 같은 경우에는 그대로 보존하면 좋을 것 같거든요.
- "그럼에도 불구하고"가 적용이 되어야 할 것 같습니다. "가독성을 해침에도 불구하고" 필수불가결한 성능 개선 포인트다!! 라는게 입증이 되면 best일 것 같습니다.
말이 길어졌는데, 추후에 한번 같이 오프/온라인으로 얘기해보면 좋을 것 같아요!
performace 팀끼리도 한번 같이 논의해보시지요!! (issue로 논의하는 것도 환영합니다!)
| return [ | ||
| Array.from(clocMap, ([dateString, value]) => ({ dateString, value })), | ||
| Array.from(commitMap, ([dateString, value]) => ({ dateString, value })) | ||
| ]; |
There was a problem hiding this comment.
[고민] 이 부분에서 얻는 성능적인 장점이 클지,
아니면 기존의 extract method 를 이용하여 함수 형태로 로직을 빼서 가독성을 올리고 재사용성을 올리는것이 더 의미가 있을지에 대한
trade-off를 논의해보는 것도 필요할 것 같습니다.
무작정 성능을 올리기 위해 다른 품질요구사항들을 버리게 된다면,
그냥 함수 하나를 길게 통으로 가져가버려도 괜찮다는 결론에 이르게 될 수도 있지 않을까용?
There was a problem hiding this comment.
중복된 로직으로 보여서 buildReturnArray 헬퍼함수를 삭제하지 않고 내부에서 변경된 로직을 return 하면 더 좋겠다는 생각이 들어요ㅎㅎ
| const commitMap = new Map<string, number>(); | ||
|
|
||
| sortedFilteredData.forEach(({ commit }) => { | ||
| for (const { commit } of sortedFilteredData) { |
There was a problem hiding this comment.
요 방식에서 성능적인 차이가 나는 건 몰랐네요!!! 배웠습니다!!
SingTheCode
left a comment
There was a problem hiding this comment.
가독성을 높이기 위한 추상화와 성능 사이에서 고민할 수 있었던 계기가 되어 좋았습니다ㅎㅎ 특히 Array api를 가독성이 좋다는 이유로 주로 사용하곤 했었는데 for of 가 성능 상 더 좋다는 건 처음 알았어요!!! 고생하셨습니다ㅎㅎ 아래에 몇가지 코멘트 남길게요ㅎㅎ
컴퓨터 성능이 예전보다 좋아졌기에 Array api 를 사용함으로써 성능에 큰 체감을 느낀 적은 크게 없었던 것 같아요! 물론 O(n2)이 되기 시작하면 얘기는 달라지겠지만요ㅎㅎ
최대 18.9%의 성능 개선은 작은 수치는 아니지만, 14.37ms -> 12.47ms 로 1.9ms 가 감소한 것을 보면, 역할 분리 및 중복제거를 위해 작성한 헬퍼함수를 풀만큼 체감되는 수치는 아니라는 생각이 듭니다ㅎㅎ
지금은 최대 커밋 개수가 5000개 일 때라 수치가 작게 나올 수도 있을 것 같아서 헬퍼함수를 풀어보고 묶어보고 하면서 커밋 개수가 1만개 이상일 때 유의미한 차이가 있는지 검증해보는 것도 좋을 것 같아요!
상하위 10% 이상치 제거가 어떤 의미일까요?? 잘 몰라서 여쭤봅니다ㅎㅎㅎ
| const commitMap = new Map<string, number>(); | ||
|
|
||
| sortedFilteredData.forEach(({ commit }) => { | ||
| for (const { commit } of sortedFilteredData) { |
There was a problem hiding this comment.
저는 개인적으로 Array api 가 명시적이여서 가독성을 위해 자주 사용하는 편입니다!
forEach 를 for of 로 변경했을 때 가독성을 조금 덜 신경쓸만큼의 유의미한 성능차이가 나는지 검증이 포함되면 좋을 것 같습니다ㅎㅎ
| return [ | ||
| Array.from(clocMap, ([dateString, value]) => ({ dateString, value })), | ||
| Array.from(commitMap, ([dateString, value]) => ({ dateString, value })) | ||
| ]; |
There was a problem hiding this comment.
중복된 로직으로 보여서 buildReturnArray 헬퍼함수를 삭제하지 않고 내부에서 변경된 로직을 return 하면 더 좋겠다는 생각이 들어요ㅎㅎ
test를 하면 모종(?)의 이유들로 수치들이 튈수가 있어서, outlier라고 생각되는 놈들을 제거하는 거라고 보면 될 것 같습니다. 저는 실험 방법이 참 좋았던게, 보통 3번 정도 돌린 다음에 평균을 내고 끝내는데 하지만, 우리의 시간도 소중하니까 ㅎㅎㅎㅎ 보통의 케이스에서는 3번 평균 정도로 해도 충분한 결과를 낼 수 있을 것 같습니다. |
|
아, 참고로 말머리를 refactor 로 바꿨습니다. |
chae-dahee
left a comment
There was a problem hiding this comment.
추후 작업으로 Issue #881 에서 코드리뷰 달린 논의와 리팩토링 진행하겠습니다!
TemporalFilter 라인 차트 데이터 처리 성능 최적화
요약
TemporalFilter 컴포넌트의 라인 차트 데이터 처리 성능을 루프 연산과 데이터 변환 최적화를 통해 개선했습니다. 일반적인 저장소 크기에서 약 10-20% 더 빠른 렌더링 속도를 달성했습니다.
변경사항
forEach를for-of루프로 교체||연산자)Array.from매핑 함수 사용Related issue
#835
Result
📊 성능 개선 결과
테스트 환경
1,000 커밋 데이터셋 분석 (일반적 사용 케이스)
변동계수: 93% 개선 (더 안정적)
</details>
Work list
forEach를for-of루프로 교체Array.from직접 매핑 적용Discussion
💡 왜 이 최적화가 효과적인가?
for-of루프:forEach와 달리 함수 컨텍스트를 생성하지 않아 오버헤드가 적습니다📋 테스트 방법
🔍 코드 변경 전후 비교
<details> <summary>코드 비교 보기</summary>Before:
After:
</details>
모든 테스트를 통과했으며, 실제 사용 환경에서도 안정적인 성능 향상을 확인했습니다. 리뷰 부탁드립니다! 🙏