-
Notifications
You must be signed in to change notification settings - Fork 168
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
メモリバッファクラスをx64対応できるように書き替える #1618
メモリバッファクラスをx64対応できるように書き替える #1618
Conversation
❌ Build sakura 1.0.3646 failed (commit e869c6f2d4 by @berryzplus) |
❌ Build sakura 1.0.3647 failed (commit 0fdb081204 by @berryzplus) |
サイズ変数をsize_t化した際の修正が不十分でVA発生いたため、追加修正
✅ Build sakura 1.0.3648 completed (commit 49e448e076 by @berryzplus) |
✅ Build sakura 1.0.3649 completed (commit 0f678d8dac by @berryzplus) |
こんなの出てたので追加修正しました。
|
❌ Build sakura 1.0.3650 failed (commit f2d01f74b0 by @berryzplus) |
✅ Build sakura 1.0.3651 completed (commit 341a66af3a by @berryzplus) |
#1618 (comment) を対応しようとしたらこんなの出たので諦めます。
|
Kudos, SonarCloud Quality Gate passed! |
✅ Build sakura 1.0.3652 completed (commit 839aa7ff0e by @berryzplus) |
CMemory cmem( pData, nDataLen ); | ||
cmem.SwapHLByte(); | ||
if( cmem.GetRawPtr() != nullptr ){ | ||
::memcpy( pData, cmem.GetRawPtr(), nDataLen ); |
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.
元のコードが過去にパフォーマンス問題に対策した結果のものに見えるので、115行目のコンストラクタと memcpy での2回のコピーがパフォーマンスに与える影響が気になりました。ビット演算するのが書いた人の趣味だった可能性もありますけれど…。
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.
bswapはCPU命令なので、パフォーマンスを重視するならこちらを使うべきです。
もともとはstaticバージョンに本体があり、インスタンスメソッド側が参照する構造になっていたのを逆転しています。staticバージョンを使わない方向に持っていき、最終的には廃止したいと考えているので、あえて不都合が起きる実装にしています。いきなり動かなくすると困りそうなのでいったん残してある状態です。
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.
過渡的な実装ということで承知しました。
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.
ちなみにstaticバージョンでメモリコピーを行っている理由は、以下です。
- bswapの対象は
uint16_t(=unsigned short)
です。uint16_t*
の値は、必ず偶数アドレスになります。
- static関数の引数は
char*
です。char*
の値は、奇数アドレスになりえます。
👇
- bswapを使うには、
char*
をuint16_t*
に reinterpret キャストする必要があるので、問題をふせぐために新たに確保したメモリにコピーしてから処理します。
細かいことをいうと、mallocが返すアドレス値がどんな値であるか(≒偶数or奇数、アラインメント単位は?)について言語仕様的な規定はないはずです。返却されるメモリアドレスになんらかの要求仕様があるときには _aligned_malloc を使って仕様を明示します。一応、mallocで奇数アドレスが返ってきたのを見たことありませんし、現状の通常mallocで問題はないと思っていますが。
p[1] = ctemp; | ||
// 既に必要サイズを確保できている場合、直ちに抜ける | ||
if( nAllocSize < m_nDataBufSize ){ | ||
return; |
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.
不具合ではないのですが、<
よりも<=
のほうがほんの少しだけ幸せになるかもしれないと思いました。
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.
ここを less than にした理由は capacity() の実装に起因しています。
おっしゃる通りだと思うのでどこかで対応したいです。(今回は見送りたいです)
void _SetRawLength( size_t nLength ); | ||
void swap( CMemory& left ) noexcept; | ||
//! メモリ再確保を行わずに格納できる最大バイト数を求める | ||
[[nodiscard]] int capacity() const noexcept { return 8 <= m_nDataBufSize ? m_nDataBufSize - 2: 0; } |
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.
この8というマジックナンバーが曲者ですね。
2を引くのはm_nDataBufSizeがNUL終端を含んでいるからというのが根拠ですが、
8のほうはAllocBufferの実装に強く依存している可能性があるというか。
マイナス値を避けたいので変更したんだと思いますが、
2以上であれば、8でなくてもよいというなら、2のほうがいいかもしれないとは思います。
(これは修正要求ではないので、変更しなくてもかまいません)
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.
マジックナンバー「8」についてはクラスコメントに記載しました。
「内部バッファのサイズは8の倍数に切り上げて確保される。」
8を定数化するのがあるべき姿かもしれません。
(冷静に考えると、0より1つ大きい値が8なので「0 < m_nDataBufSize」にしとけばいいのかも。)
現在のcapacity()実装は標準ライブラリstd::wstring::capacity()と異なるので、メソッドをリネームするか、実装を標準ライブラリに寄せるかして取り違え防止を図るのがよいと思います。
2 = sizeof(wchar_t) だから実質的に「1つ違いエラー」の見直しになるので今回PR対象からは外しました。
レビューありがとうございます。とりあえずマージしちゃいます。 このPRは目的が「準備」であることもあり、至らぬ点が多数ある認識っす。 何か気付いた点があれば、このPRのコメントでも良いですし、新規issueでも良いので課題をあげてもらえれば幸いです。 |
PR の目的
CMemoryをリファクタリングして、x64対応作業を始められるようにすることが目的です。
カテゴリ
PR の背景
CMemoryやCNativeWはサクラエディタの中核に位置するクラスです。
SonarCloudによる静的解析で、以下のBugsレベル警告が検出されています。
これらのクラスは、依存関係のルート付近にあるヘッダーで定義されているため、修正がしづらいです。
最近マージされた #1615 により、修正のやりづらさはさらに増したと思います。
ただ、ここを修正しないと x64対応 は不可能なので、どこかでなんとかしたいと考えていました。
今回は、思い切ってCMemory/CNativeWの大幅改修をして、x64対応の作業ができる状態を作り出したいと思います。
PR のメリット
※従来は異常を検出できず、処理が続行されていました。
x64 - Debug
ビルドの警告が 576件 から 497件 に減ります。PR のデメリット (トレードオフとかあれば)
仕様・動作説明
サクラエディタの仕様・機能に変更はありません。
CMemoryで定義されているメソッドの改廃を行っています。
Windows SDK
の_swabを利用するためには、好ましくないキャストをする必要がある。加えて、_swabを利用するメリットはとくにないため。PR の影響範囲
テスト内容
影響範囲が広すぎるので、単純な起動確認をしたほうが良さそうです。
関連 issue, PR
#1504
#1541
#1575
#1605
参考資料