Page MenuHomeWildfire Games
Differential D4867

Adds unlikely to ENSURE
Needs ReviewPublic
Actions

Authored by vladislavbelov on Jan 6 2023, 9:45 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

We use ENSURE as a runtime check of an error that might break something. Usually we shouldn't enter that code.

Ideally we should use PGO, but we don't. So as experiment I added unlikely.

Test Plan
  1. Apply the patch and compile the game
  2. Compare performance for some CPU heavy scene/replay

Diff Detail

Event Timeline

vladislavbelov created this revision.Jan 6 2023, 9:45 PM
Vulcan added a comment.Jan 6 2023, 10:10 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/6634/display/redirect

Vulcan added a comment.Jan 6 2023, 10:10 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/8333/display/redirect

Vulcan added a comment.Jan 6 2023, 10:24 PM

Successful build - Chance fights ever on the side of the prudent.

Debug:
     9>tinygettext.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\tinygettext.vcxproj]
    17>wsecure_crt.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>wprofiler.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>manifest.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>secure_crt.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>posix.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>vfs_path.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>file_stats.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/7726/display/redirect

Harbormaster completed remote builds in B21341: Diff 21275.Jan 6 2023, 10:24 PM
vladislavbelov requested review of this revision.Jan 6 2023, 10:24 PM
Stan added a subscriber: Stan.EditedJan 6 2023, 10:28 PM

Looks like EOL issues... Patch sounds good though. Did you see any difference?

vladislavbelov updated this revision to Diff 21276.Jan 6 2023, 10:29 PM
In D4867#207781, @Stan wrote:

Looks like EOL issues.

Fixed.

Did you see any difference?

MSVC doesn't support it before C++20.

Stan added a subscriber: phosit.Jan 6 2023, 10:32 PM

cc @phosit

Vulcan added a comment.Jan 6 2023, 11:03 PM

Successful build - Chance fights ever on the side of the prudent.

Debug:
     8>tinygettext.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\tinygettext.vcxproj]
    17>wsecure_crt.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>wprofiler.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>manifest.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>secure_crt.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>posix.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>vfs_path.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]
    17>file_stats.obj : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\lowlevel.vcxproj]

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/7727/display/redirect

Vulcan added a comment.Jan 6 2023, 11:36 PM

Successful build - Chance fights ever on the side of the prudent.

builderr-debug-clang8.txt
In file included from ../../../source/pch/gui/precompiled.h:27:
In file included from ../../../source/gui/ObjectBases/IGUIObject.h:29:
../../../source/gui/SettingTypes/CGUIHotkey.h:37:2: warning: explicitly defaulted move assignment operator is implicitly deleted [-Wdefaulted-function-deleted]
        MOVABLE(CGUIHotkey);
        ^
../../../source/lib/code_annotation.h:237:13: note: expanded from macro 'MOVABLE'
        className& operator=(className&&) = default
                   ^
../../../source/gui/SettingTypes/CGUIHotkey.h:31:20: note: move assignment operator of 'CGUIHotkey' is implicitly deleted because base class 'CGUISimpleSetting<CStr8>' has a deleted move assignment operator
class CGUIHotkey : public CGUISimpleSetting<CStr>
                   ^
../../../source/gui/CGUISetting.h:91:2: note: 'operator=' has been explicitly marked deleted here
        NONCOPYABLE(CGUISimpleSetting);
        ^
../../../source/lib/code_annotation.h:229:13: note: expanded from macro 'NONCOPYABLE'
        className& operator=(const className&) = delete
                   ^
1 warning generated.
In file included from ../../../source/gui/GUIObjectTypes.cpp:30:
../../../source/gui/ObjectTypes/COList.h:36:2: warning: explicitly defaulted move assignment operator is implicitly deleted [-Wdefaulted-function-deleted]
        MOVABLE(COListColumn);
        ^
../../../source/lib/code_annotation.h:237:13: note: expanded from macro 'MOVABLE'
        className& operator=(className&&) = default
                   ^
../../../source/gui/ObjectTypes/COList.h:41:30: note: move assignment operator of 'COListColumn' is implicitly deleted because field 'm_List' has a deleted move assignment operator
        CGUISimpleSetting<CGUIList> m_List;
                                    ^
../../../source/gui/CGUISetting.h:91:2: note: 'operator=' has been explicitly marked deleted here
        NONCOPYABLE(CGUISimpleSetting);
        ^
../../../source/lib/code_annotation.h:229:13: note: expanded from macro 'NONCOPYABLE'
        className& operator=(const className&) = delete
                   ^
1 warning generated.
In file included from ../../../source/gui/ObjectTypes/COList.cpp:20:
../../../source/gui/ObjectTypes/COList.h:36:2: warning: explicitly defaulted move assignment operator is implicitly deleted [-Wdefaulted-function-deleted]
        MOVABLE(COListColumn);
        ^
../../../source/lib/code_annotation.h:237:13: note: expanded from macro 'MOVABLE'
        className& operator=(className&&) = default
                   ^
../../../source/gui/ObjectTypes/COList.h:41:30: note: move assignment operator of 'COListColumn' is implicitly deleted because field 'm_List' has a deleted move assignment operator
        CGUISimpleSetting<CGUIList> m_List;
                                    ^
../../../source/gui/CGUISetting.h:91:2: note: 'operator=' has been explicitly marked deleted here
        NONCOPYABLE(CGUISimpleSetting);
        ^
../../../source/lib/code_annotation.h:229:13: note: expanded from macro 'NONCOPYABLE'
        className& operator=(const className&) = delete
                   ^
1 warning generated.
builderr-release-clang8.txt
In file included from ../../../source/pch/gui/precompiled.h:27:
In file included from ../../../source/gui/ObjectBases/IGUIObject.h:29:
../../../source/gui/SettingTypes/CGUIHotkey.h:37:2: warning: explicitly defaulted move assignment operator is implicitly deleted [-Wdefaulted-function-deleted]
        MOVABLE(CGUIHotkey);
        ^
../../../source/lib/code_annotation.h:237:13: note: expanded from macro 'MOVABLE'
        className& operator=(className&&) = default
                   ^
../../../source/gui/SettingTypes/CGUIHotkey.h:31:20: note: move assignment operator of 'CGUIHotkey' is implicitly deleted because base class 'CGUISimpleSetting<CStr8>' has a deleted move assignment operator
class CGUIHotkey : public CGUISimpleSetting<CStr>
                   ^
../../../source/gui/CGUISetting.h:91:2: note: 'operator=' has been explicitly marked deleted here
        NONCOPYABLE(CGUISimpleSetting);
        ^
../../../source/lib/code_annotation.h:229:13: note: expanded from macro 'NONCOPYABLE'
        className& operator=(const className&) = delete
                   ^
1 warning generated.
In file included from ../../../source/gui/GUIObjectTypes.cpp:30:
../../../source/gui/ObjectTypes/COList.h:36:2: warning: explicitly defaulted move assignment operator is implicitly deleted [-Wdefaulted-function-deleted]
        MOVABLE(COListColumn);
        ^
../../../source/lib/code_annotation.h:237:13: note: expanded from macro 'MOVABLE'
        className& operator=(className&&) = default
                   ^
../../../source/gui/ObjectTypes/COList.h:41:30: note: move assignment operator of 'COListColumn' is implicitly deleted because field 'm_List' has a deleted move assignment operator
        CGUISimpleSetting<CGUIList> m_List;
                                    ^
../../../source/gui/CGUISetting.h:91:2: note: 'operator=' has been explicitly marked deleted here
        NONCOPYABLE(CGUISimpleSetting);
        ^
../../../source/lib/code_annotation.h:229:13: note: expanded from macro 'NONCOPYABLE'
        className& operator=(const className&) = delete
                   ^
1 warning generated.
In file included from ../../../source/gui/ObjectTypes/COList.cpp:20:
../../../source/gui/ObjectTypes/COList.h:36:2: warning: explicitly defaulted move assignment operator is implicitly deleted [-Wdefaulted-function-deleted]
        MOVABLE(COListColumn);
        ^
../../../source/lib/code_annotation.h:237:13: note: expanded from macro 'MOVABLE'
        className& operator=(className&&) = default
                   ^
../../../source/gui/ObjectTypes/COList.h:41:30: note: move assignment operator of 'COListColumn' is implicitly deleted because field 'm_List' has a deleted move assignment operator
        CGUISimpleSetting<CGUIList> m_List;
                                    ^
../../../source/gui/CGUISetting.h:91:2: note: 'operator=' has been explicitly marked deleted here
        NONCOPYABLE(CGUISimpleSetting);
        ^
../../../source/lib/code_annotation.h:229:13: note: expanded from macro 'NONCOPYABLE'
        className& operator=(const className&) = delete
                   ^
1 warning generated.
builderr-release-gcc7.txt
In member function 'void CInput::UpdateText(int, int, int)':
cc1plus: warning: 'void* __builtin_memset(void*, int, long unsigned int)': specified size 18446744073709551612 exceeds maximum object size 9223372036854775807 [-Wstringop-overflow=]

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/8334/display/redirect

Vulcan added a comment.Jan 6 2023, 11:59 PM

Build failure - The Moirai have given mortals hearts that can endure.

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libnetwork_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libtinygettext_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libtinygettext_dbg.a(tinygettext.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/liblobby_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libglooxwrapper_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libsimulation2_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libscriptinterface_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libengine_dbg.a(precompiled.o) has no symbols
../../../source/graphics/ShaderManager.cpp:271:26: error: call to unavailable member function 'value': introduced in macOS 10.14
        XERO_ITER_EL(usableTech.value(), Child)
                     ~~~~~~~~~~~^~~~~
../../../source/ps/XML/Xeromyces.h:93:34: note: expanded from macro 'XERO_ITER_EL'
        for (XMBElement child_element : parent_element.GetChildNodes())
                                        ^~~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/optional:942:27: note: candidate function has been explicitly made unavailable
    constexpr value_type& value() &
                          ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/optional:933:33: note: candidate function has been explicitly made unavailable
    constexpr value_type const& value() const&
                                ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/optional:951:28: note: candidate function not viable: no known conversion from 'optional<...>' to 'optional<...>' for object argument
    constexpr value_type&& value() &&
                           ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/optional:960:34: note: candidate function not viable: no known conversion from 'optional<...>' to 'const optional<...>' for object argument
    constexpr value_type const&& value() const&&
                                 ^
../../../source/graphics/ShaderManager.cpp:287:26: error: call to unavailable member function 'value': introduced in macOS 10.14
        XERO_ITER_EL(usableTech.value(), Child)
                     ~~~~~~~~~~~^~~~~
../../../source/ps/XML/Xeromyces.h:93:34: note: expanded from macro 'XERO_ITER_EL'
        for (XMBElement child_element : parent_element.GetChildNodes())
                                        ^~~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/optional:942:27: note: candidate function has been explicitly made unavailable
    constexpr value_type& value() &
                          ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/optional:933:33: note: candidate function has been explicitly made unavailable
    constexpr value_type const& value() const&
                                ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/optional:951:28: note: candidate function not viable: no known conversion from 'optional<...>' to 'optional<...>' for object argument
    constexpr value_type&& value() &&
                           ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/optional:960:34: note: candidate function not viable: no known conversion from 'optional<...>' to 'const optional<...>' for object argument
    constexpr value_type const&& value() const&&
                                 ^
2 errors generated.
make[1]: *** [obj/graphics_Debug/ShaderManager.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [graphics] Error 2

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/6635/display/redirect

Harbormaster completed remote builds in B21342: Diff 21276.Jan 6 2023, 11:59 PM
phosit added a comment.Jan 7 2023, 11:35 AM

without attribute:

    34326,18 msec task-clock:u              #    1,068 CPUs utilized
           0      context-switches:u        #    0,000 K/sec
           0      cpu-migrations:u          #    0,000 K/sec
      229620      page-faults:u             #    0,007 M/sec
106205906507      cycles:u                  #    3,094 GHz
143643397815      instructions:u            #    1,35  insn per cycle  
 27710251992      branches:u                #  807,263 M/sec
   377210643      branch-misses:u           #    1,36% of all branches
 
32,132345683 seconds time elapsed
 
33,718835000 seconds user
 0,653523000 seconds sys

with attribute:

    33793,66 msec task-clock:u              #    1,069 CPUs utilized          
           0      context-switches:u        #    0,000 K/sec                  
           0      cpu-migrations:u          #    0,000 K/sec                  
      225073      page-faults:u             #    0,007 M/sec                  
105864200013      cycles:u                  #    3,133 GHz                    
143493761916      instructions:u            #    1,36  insn per cycle         
 27668264777      branches:u                #  818,741 M/sec                  
   381279536      branch-misses:u           #    1,38% of all branches        

31,622717428 seconds time elapsed

33,182431000 seconds user
 0,641270000 seconds sys

that's a nonvisual replay on combat-demo-huge.
I expected a drop in branch-misses
I compiled with GCC-12

vladislavbelov added a comment.Jan 7 2023, 1:11 PM
In D4867#207806, @phosit wrote:

I expected a drop in branch-misses

Yeah, though other metrics are better. Is it stably reproducible? Because currently it looks like as a sampling artifact, as ENSURE isn’t triggered.

phosit added a comment.Jan 7 2023, 1:37 PM
In D4867#207828, @vladislavbelov wrote:

other metrics are better.

Yes, note that the frequency was bigger in the second run

Is it stably reproducible?

Ahm... no
Do you have a special (more deterministic) function I should test?

I think when it's not noticeable in benchmark it will not be noticeable by the players and not be worth implementing.

vladislavbelov added a comment.Jan 7 2023, 1:43 PM
In D4867#207829, @phosit wrote:

Do you have a special (more deterministic) function I should test?

What's about the visual replay?

I think when it's not noticeable in benchmark it will not be noticeable by the players and not be worth implementing.

Ideally it's true but usually it's more complicated.

phosit added a comment.Jan 7 2023, 2:18 PM
In D4867#207830, @vladislavbelov wrote:

What's about the visual replay?

that's even more noisy

phosit removed a subscriber: phosit.Oct 15 2023, 6:44 PM

Revision Contents
Changeset List

PathSize
source/
lib/
12 lines
6 lines

Diff 21276

View Options

source/lib/code_annotation.h

Loading...
View Options

source/lib/debug.h

Loading...
Wildfire Games · Phabricator