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

[성능 베이스캠프 미션] 온스타(온승찬) 미션 제출합니다. #64

Merged
merged 14 commits into from
Sep 7, 2022

Conversation

onschan
Copy link

@onschan onschan commented Sep 5, 2022

🔥 결과

/_ 개선 목표에 있는 측정 항목들에 대해 개선 작업 전/후의 성능 측정 결과를 적어주세요. _/

스크린샷 2022-09-06 14 38 18

스크린샷 2022-09-06 15 41 56

✅ 개선 작업 목록

/_ 각 요구사항을 위해 어떤 개선 작업을 진행했는지 적어주세요
코드 변경사항으로 확인하기 어려운 CloudFront 설정 사항 등은 리뷰어가 확인할 수 있게 스크린샷이나 적용한 항목들을 적어주면 좋겠지요? 🙂
_/

1 요청 크기 줄이기

  • 소스코드 크기 줄이기

development 모드
스크린샷 2022-09-06 14 21 25

production 모드
스크린샷 2022-09-06 14 23 32
기 줄이기

웹팩의 production의 분리만으로도 기본적인 최적화가 된다고하여(terser 자동 적용 등) 개발 환경과 배포환경을 분리하여 웹팩 설정에 따라 빌드 될 수 있도록 하였습니다.

배포 환경에서는 플러그인을 통해 css, js의 번들크기를 줄여주었고, 임의의 해시값을 넣어주어 배포시 자동으로 최신 배포 파일을 바라볼 수 있도록 하였습니다. 추가적으로 gzip을 통한 압축파일을 생성하였으나, cloudfront에서도 해준다고하여, 해당 부분이 유효한지는 아직 의문입니다.

  • 이미지 크기 줄이기

png

png 파일은 webp로 변환하여 용량을 줄여주었고, webp 확장자로만 변환하여 사용하였습니다.
webp가 지원안되는 브라우저도 지원을 할까하다가 webp는 현재 IE 말고는 모든 브라우저에서 호환이 가능하기때문에, 호환을 해주지 않는 편이 IE 사용자를 위한 일이라는 생각하여 webp를 지원하지 않는 환경은 고려하지 않도록하였습니다.

gif

gif는 비디오 타입을 이용하는 것이 용량적으로도 훨씬 유리하여 mp4 파일로 변환하여 사용하였습니다. wemb으로도 변환한 파일을 넣었다가 mp4와 webm의 용량 차이가 미미하고 지원안되는 브라우저가 wemp에 비해 더 존재하여 mp4로만 진행하였습니다.

2 필요한 것만 요청하기

  • 페이지별 리소스 분리
    리액트의 code split과 lazy 기능을 이용하여 페이지별 리소스를 분리하였습니다.

  • 아이콘 패키지 Tree Shaking
    @react-icon/all-files 를 사용했는데, 해당 플러그인을 사용하지 않아도 같은 효과가 난다고 하여 좀 더 살펴본 뒤 수정할 예쩡입니다.

3 같은 건 매번 새로 요청하지 않기

  • CloudFront 캐시 설정 (설정값, 해당 값을 설정한 이유 포함)

스크린샷 2022-09-06 14 40 54

스크린샷 2022-09-06 14 42 38

스크린샷 2022-09-06 14 42 43

control-cache의 max-age를 1년으로 설정하여 사용하였습니다. 대부분의 환경에서 1년을 사용하고 있고, 안정적이라하여 사용하였습니다.

  • GIPHY의 trending API를 Search 페이지에 들어올 때마다 새로 요청하지 않아야 한다.
    Map을 생성하여 API 콜에대한 key,value로 관리하고있습니다. 추가적인 캐싱전략은 따로 세우지 않았습니다.

4 최소한의 변경만 일으키기

  • 검색 결과 > 추가 로드시 추가된 목록만 새로 렌더되어야 한다.
  • Layout Shift 없이 애니메이션이 일어나야 한다.
  • Frame Drop이 일어나지 않아야 한다.
    • (Chrome DevTools 기준) Partially Presented Frame 역시 최소로 발생해야 한다.

🧐 공유

/_ 작업하면서 든 생각, 질문, 새롭게 학습하거나 시도해본 내용 등등 공유할 사항이 있다면 자유롭게 적어주세요 _/

@onschan onschan marked this pull request as ready for review September 6, 2022 05:20
Copy link

@LAH1203 LAH1203 left a comment

Choose a reason for hiding this comment

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

안녕하세요 온스타~! 제가 온스타의 리뷰어가 되다니 정말 영광입니다,,⭐️

페이지의 성능이 정말 많이 좋아졌네요! 많이 신경 써주신 것 같아요 굿굿 👏👏

우선 저희의 요구사항들부터 먼저 볼까요? 적어주신 걸 코드와 비교하면서 봤는데, 몇 가지 궁금한 점이 생겨서 여쭤보려고 합니다 ㅎㅎ

  1. 빌드 시 source-map 옵션

소스코드 크기 줄이기에서 남겨주신 사진을 보니 production mode로 빌드할 경우에도 source-map을 만들도록 옵션을 주신 것 같더라고요! 혹시 그렇게 하신 이유가 있을까요?

(번외... webp는 현재 IE 말고는 모든 브라우저에서 호환이 가능하기때문에, 호환을 해주지 않는 편이 IE 사용자를 위한 일이라는 생각하여 webp를 지원하지 않는 환경은 고려하지 않도록하였습니다. → ㅋㅋㅋㅋㅋㅋㅋㅋㅋ잔인해!)

  1. 캐시 저장소 전략

코드에서 Map으로 직접 구현해주신 캐시 저장소를 보면 따로 만료 시간이 정해져 있지는 않아요. 만약에 사용자가 페이지를 끄지 않은 상태로 며칠동안 두었을 때도 똑같이 유지된다면, 사용자가 이를 정말 trending gif라고 판단할 수 있을까요? 저도 리뷰를 받고 시간을 설정해주는 편이 좋을 것 같다고 생각하여 설정해주었는데, 이에 대해 온스타의 생각도 궁금해서 리뷰 남겨봐요!

  1. react_devtools_backend.js

올려주신 페이지를 lighthouse로 성능 측정을 해봤을 때 퍼포먼스에서 뜨는 경고가 react_devtools_backend.js에 관련된 것이었어요. 저는 devtool이라 production에서는 사용하지 않을 것이라고 판단하여 제거해주었는데, 온스타는 남겨주셨더라고요! 혹시 이에 대해서도 의견을 물어볼 수 있을까요?

그 외에 궁금한 점은 코드에 코멘트로 남겨봤어요~ 😊

Comment on lines +7 to +11
"build": "webpack --config webpack.dev.js",
"build:dev": "webpack --config webpack.dev.js",
"build:prod": "webpack --config webpack.prod.js",
"watch": "webpack --watch",
"serve": "webpack serve --mode=development",
"serve": "webpack serve --config webpack.dev.js",
Copy link

Choose a reason for hiding this comment

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

오호 아예 웹팩 설정까지 나눠주셨군요! 좋아요 👏

"webpack": "^5.50.0",
"webpack-cli": "^4.7.2",
"webpack-dev-server": "^3.11.2"
"webpack-cli": "^4.10.0",
Copy link

Choose a reason for hiding this comment

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

webpack-cli는 버전에 변경이 있는데 이유가 있을까요~?

Copy link
Author

@onschan onschan Sep 7, 2022

Choose a reason for hiding this comment

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

webpack 관련 플러그인 설치 과정 중 cli 버전 업데이트 할래? 라는 안내가 떠서 y 를 눌렀더니 업데이트가 되었습니다 ㅎㅎ..
아쉽게도 딱히 제가 더 설명드릴 이유는 없어요 😂

package.json Outdated
"@babel/core": "^7.15.0",
"@babel/plugin-transform-runtime": "^7.15.0",
"@babel/preset-env": "^7.15.0",
"@babel/preset-react": "^7.14.5",
"@react-icons/all-files": "^4.1.0",
Copy link

Choose a reason for hiding this comment

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

원래 react-icons는 dependencies에 있다가 @react-icons/all-files는 devDependencies로 옮겨갔네요! dev로 설치해주신 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 라이브러리는 build될 때에만 사용될 부분이다라고 생각해서 dev로 빼놨는데, production 코드에도 결국 적용되는 거니깐 dependency가 더 어울리는 것 같기도허구 살짝 긴가민가하네요.
하리는 해당 부분 처럼 아이콘 라이브러리는 어느 dependency가 더 어울리다고 생각하시나요?

@react-icons/all-files 대신 react-icons으로 다시 바꿀 에정이긴 합니다!

Copy link

Choose a reason for hiding this comment

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

저는 icons는 실제 빌드된 결과물에서도 보여져야 하는 거라고 생각해서 devDependencies보다는 dependencies에 좀 더 어울리는 것 같다는 생각은 있어요! (전 헷갈릴 때 보통 다른 사람들은 어떻게 설치하는지를 많이 참고하고는 합니다 ㅎㅎ)

src/App.tsx Outdated
Comment on lines 9 to 11
import Home from './pages/Home/Home';

const Search = React.lazy(() => import('./pages/Search/Search'));
Copy link

Choose a reason for hiding this comment

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

만약에 사용자가 Home 페이지가 아니라 Search 페이지에서 먼저 접속한다면 어떻게 될까요?

Copy link
Author

Choose a reason for hiding this comment

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

Search 페이지로 들어올때도 home에 해당하는 js 파일을 받아오겠군요..!
다만, home에 대해서도 lazy 적용했을때 성능 점수가 떨어지는 일이 발생했는데, 혹시 이에 대해 하리가 아는 이유가 있을까요?

Copy link

Choose a reason for hiding this comment

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

음.. lazy 적용한 Home에 대한 요청이 한번 더 가서 그런걸까요? 저도 그 부분은 테스트해보지 않아서 명확한 이유는 잘 모르겠네요.. 😅

Comment on lines +15 to +23
async function cacheApi(api: string, key: string) {
if (cacheData.has(key)) {
return cacheData.get(key);
}

await fetch(api).then((res) => cacheData.set(key, res.json()));

return cacheData.get(key);
}
Copy link

Choose a reason for hiding this comment

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

크 직접 구현해주신거 너무 좋습니다~ 👏👍👏

Comment on lines +43 to +45
<FeatureItem title="See trending gif" videoSrc={trendingVideo} />
<FeatureItem title="Find gif for free" videoSrc={findVideo} />
<FeatureItem title="Free for everyone" videoSrc={freeVideo} />
Copy link

Choose a reason for hiding this comment

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

와 props 이름 디테일까지 챙겨주셨네요... 리스펙 🙏

Copy link
Author

Choose a reason for hiding this comment

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

다 하리한테서 배운겁니다😄

Copy link

Choose a reason for hiding this comment

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

세상에... 😮

@@ -37,18 +26,37 @@ module.exports = {
},
{
test: /\.css$/i,
use: ['style-loader', 'css-loader']
use: [MiniCssExtractPlugin.loader, 'css-loader']
Copy link

Choose a reason for hiding this comment

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

style-loader를 빼고 MiniCssExtractPlugin을 적용시켜서 빌드 시 css 파일이 따로 나올 수 있도록 만들어주신 것 같은데, 이럴 경우 장점이 뭘까요? 그리고 style-loader를 아예 빼신 이유도 알 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

css-loader 를 통해 번들된 css를 style 태그로 넣어주는 역할을 하는데,
이때 모든 css 코드를 한번에 다루기 때문에, 압축된 css 파일을 추출하여 필요한 부분에 해당하는 css 파일만 사용하는 것이 불가능해 css 성능 최적화와는 어울리지 않는다는 단점이 있었습니다.

따라서 필요한 부분에 해당하는 css만 파일로 따로 추출하고 불러오는 것을 도와주는 mini-css-extract-plugin 플러그인을 사용하게 되었습니다.

mini-css-extract-plugin은 css 파일을 여러개 추출하는 과정을 거치기 때문에 개발 환경에서는 style-loader의 동작이 조금 더 빠르다고는 하는데,
dist 폴더를 관리하는 부분을 보시면 아시겠지만, 개발,배포 환경에 상관없이 js, css, static로 분리되어 파일들을 관리하고있어서 개발모드에서도 style-loader 보다는 mini-css-extract-plugin를 으로 사용하는 편이 더 좋겠다고 생각하여 교체하여 사용했습니다.

style-loader와 mini-css-extract-plugin를 같이 사용할 수 없고 사용하지 말라는 webpack 문서의 설명도 있었어서 style-loader는 아예 제외한 이유도 있습니다.

Copy link

Choose a reason for hiding this comment

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

크 바로 이해됐습니다. 설명 감사합니다 ⭐️

Comment on lines +32 to +33
test: /\.(eot|ttf|woff|woff2)$/i,
loader: 'url-loader',
Copy link

Choose a reason for hiding this comment

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

file-loaderurl-loader로 바꾸면서 무슨 차이가 있었나요? 궁금하네요 🥸

Copy link
Author

Choose a reason for hiding this comment

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

file-loader는 단지 자체를 읽고 복사해주는 역할을 하게되는데, url-loader는 작은 이미지나 글꼴 파일은 복사하지 않고 문자열 형태로 변환하여 파일에 넣어주는 역할을 한다고하네요.
Data URL Scheme 요거를 이용해서 변환하고 사용한다고 하는데 자세한 변환 과정에 대해서는 깊이 이해하진 못했습니다..(자세한 설명을 적기엔 지식이 부족하네요..)
다만 얕게 이해한 결과 파일의 크기가 작다면 url-loader를 이용하는 편이 효율적이겠다 싶었고 test에 font 파일들만 타겟으로 하여 사용하였습니다. url-loader 옵션 중 용량에 대한 limit을 걸어줄 수 도 있는데, font 자체가 제한선을 넘는 경우가 거의 없다 판단하여 해당 옵션은 상요하지 않았습니다.

폰트를 제외한 이미지 등의 파일은 file-loader를 사용했습니다.

webpack.prod.js Outdated
Comment on lines 10 to 22
output: {
filename: 'js/[name].[contenthash:8].js',
path: path.join(__dirname, '/dist'),
clean: true
},
devtool: 'nosources-source-map',
optimization: {
minimizer: ['...', new CssMinimizerPlugin()]
},
plugins: [
new MiniCssExtractPlugin({
filename: 'css/[name].[contenthash:8].css'
}),
Copy link

Choose a reason for hiding this comment

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

앞서 본 이미지나 폰트, development mode의 파일이랑 달리 production mode의 js와 css에만 해시를 적용해주셨네요! 이렇게 배포 모드의 js와 css에만 해시를 적용하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

파일에 임의의 해시값을 넣어주는 부분이 자동 무효화등을 위한 캐싱을 피하기 위한 이유라고 생각되는데, 개발 환경에서 과연 해시값이 필요할까라고 생각할때, 앞서 제가 이해한 바에 따르면 개발 모드에서는 필요가 없다고 느껴서 배포 환경에서만 해시값을 적용하였습니다.

Comment on lines +23 to +28
new CompressionPlugin({
algorithm: 'gzip',
test: /\.js$|\.css$/,
threshold: 10240,
minRatio: 0.8
})
Copy link

Choose a reason for hiding this comment

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

저는 cloudfront에서 제공하는 압축 옵션을 사용한터라 온스타가 말씀해주신 gzip을 통한 압축파일을 생성하였으나, cloudfront에서도 해준다고하여, 해당 부분이 유효한지는 아직 의문입니다. 이 부분이 저도 의문이기는 하네요 👀

한 가지 의견을 얹는다면 CDN이 없는 S3 자체에서는 압축하는 코드가 의미 있을 것 같아요! 실제로 저 같은 경우도 s3에서 본 번들 파일 크기와 cloudfront에서 본 번들 파일 크기가 많이 차이가 나서...ㅎㅎ
그런 의미에서는 의미가 있는 코드가 아닐까 싶어요!

Copy link
Author

Choose a reason for hiding this comment

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

오옹 좋은 의견이네요!
하나 더 얹어보자면, gzip과 brotli에 대한 검색을 하던 도중에 발견한 부분인데,
brotli가 gzip보다는 더 좋은 압축 효율을 보여주는 반면에 압축과정에서 cpu를 꽤 차지한다고하는 내용이 있었어요.
gzip이든 brotil이든 압축 과정을 웹서버에게 전부 위임한다면 웹 서버의 cpu 측면에서는 효율적이지 않을 수 있겠다는 생각이 들었는데요.
상황에 따라서 만약 웹서버가 하는 일이 많아서 부하가 걱정된다면 직접 압축하여 압축된 빌드 파일을 올리는 방법을 채택할 수도 있겠다라고 생각했습니다!

@hwangstar156
Copy link

스크린샷 2022-09-06 오후 7 04 39

ㅠㅠ ios 14이하도 Webp를 못써용.. 옛날사람 지켜!

@onschan
Copy link
Author

onschan commented Sep 7, 2022

@LAH1203
하리 꼼꼼하고 친절한 리뷰 너무 고마워요 ㅠㅠ 물어본 내용에 대해 제 의도와 아는 만큼의 설명을 덧붙여볼게요!

  1. 빌드 시 source-map 옵션
    소스코드 크기 줄이기에서 남겨주신 사진을 보니 production mode로 빌드할 경우에도 source-map을 만들도록 옵션을 주신 것 같더라고요! 혹시 그렇게 하신 이유가 있을까요?

-> 제가 source-map을 잘 이해한거인지 확신은 없지만, 번들되어 개발, 배포용으로 빌드된 파일에서 디버깅을 위해 사용되는 파일이라고 이해하고 있습니다. 개발 환경에서는 source-map이 포함되어야 개발 편의성을 위해 필요하겠지만, 배포 환경에서도 필요할지는 저도 의문이었습니다. 그러다가 https://webpack.js.org/configuration/devtool/#devtool 문서를 읽다보니, 일반적으로는 source-map을 사용하는 편이 좋지 않다고 설명되어있지만, 필요한 경우 만들어놓고 웹 서버에 올리지말거나 nosources-source-map 같은 옵션을 사용해라 라고 되어있더라고요?
배포 환경에서는 개발 모드와 다르게 nosources-source-map을 사용했는데 이 옵션이 왜 있는지 완전히 이해가 되지 않아서 직접 해보자 라는 생각으로 넣어본 옵션입니다. (아직도 이해 중.. 😅)

  1. 캐시 저장소 전략
    코드에서 Map으로 직접 구현해주신 캐시 저장소를 보면 따로 만료 시간이 정해져 있지는 않아요. 만약에 사용자가 페이지를 끄지 않은 상태로 며칠동안 두었을 때도 똑같이 유지된다면, 사용자가 이를 정말 trending gif라고 판단할 수 있을까요? 저도 리뷰를 받고 시간을 설정해주는 편이 좋을 것 같다고 생각하여 설정해주었는데, 이에 대해 온스타의 생각도 궁금해서 리뷰 남겨봐요!

-> 저도 이 부분 동감합니다! 해당 코드들을 고도화 시킨다는 가정이 있다면 캐싱 전략을 수립하는게 제일 먼저일 것같은데, 리액트 쿼리처럼 5분 정도의 캐시타임은 필요할 것 같습니다! 하리는 어떤 식으로 캐싱 전략을 수립하셨나요??

  1. react_devtools_backend.js
    올려주신 페이지를 lighthouse로 성능 측정을 해봤을 때 퍼포먼스에서 뜨는 경고가 react_devtools_backend.js에 관련된 것이었어요. 저는 devtool이라 production에서는 사용하지 않을 것이라고 판단하여 제거해주었는데, 온스타는 남겨주셨더라고요! 혹시 이에 대해서도 의견을 물어볼 수 있을까요?

-> 이거.. 어떻게 제거하나요..? 나 몰라..

@LAH1203
Copy link

LAH1203 commented Sep 7, 2022

저도 이 부분 동감합니다! 해당 코드들을 고도화 시킨다는 가정이 있다면 캐싱 전략을 수립하는게 제일 먼저일 것같은데, 리액트 쿼리처럼 5분 정도의 캐시타임은 필요할 것 같습니다! 하리는 어떤 식으로 캐싱 전략을 수립하셨나요??

  • 저는 추가적으로 한 시간정도 설정해주었어요!

이거.. 어떻게 제거하나요..? 나 몰라..

ㅋㅋㅋㅋㅋㅋㅋ 전 이거 보고 적용했습니다! facebook/react-devtools#191

Copy link

@LAH1203 LAH1203 left a comment

Choose a reason for hiding this comment

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

온스타 수고하셨습니다~ 바로 머지할게요!

@LAH1203 LAH1203 merged commit a3c8342 into woowacourse:cks3066 Sep 7, 2022
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.

3 participants