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

Deboosting #1314

Merged
merged 12 commits into from Jun 10, 2017

Conversation

Projects
None yet
3 participants
@psi29a
Member

psi29a commented Jun 9, 2017

This is an attempt to de-boost the code by using std::* equivalents. This has the effect of decreasing compile times without lose of functionality.

@psi29a psi29a added the Do not merge label Jun 10, 2017

@psi29a psi29a changed the title from Deboosting to Deboosting WIP (msvc nonesense) Jun 10, 2017

@psi29a psi29a removed the Do not merge label Jun 10, 2017

@Allofich

This comment has been minimized.

Show comment
Hide comment
@Allofich

Allofich Jun 10, 2017

Contributor

Hope this works out. It's nice to see a PR that removes more lines than it adds.

Contributor

Allofich commented Jun 10, 2017

Hope this works out. It's nice to see a PR that removes more lines than it adds.

@psi29a psi29a changed the title from Deboosting WIP (msvc nonesense) to Deboosting Jun 10, 2017

@psi29a

This comment has been minimized.

Show comment
Hide comment
@psi29a

psi29a Jun 10, 2017

Member

Me too. ;)

I did my best to test the functionality that I replaced to make sure the std::* functions work as intended. However more eyes on this branch, not just code but gameplay, that things play normally.

Member

psi29a commented Jun 10, 2017

Me too. ;)

I did my best to test the functionality that I replaced to make sure the std::* functions work as intended. However more eyes on this branch, not just code but gameplay, that things play normally.

@zinnschlag zinnschlag merged commit 11c4aed into OpenMW:master Jun 10, 2017

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Allofich

This comment has been minimized.

Show comment
Hide comment
@Allofich

Allofich Jun 10, 2017

Contributor

That was quick. Do you plan to phase out Boost usage until it's all out?

Contributor

Allofich commented Jun 10, 2017

That was quick. Do you plan to phase out Boost usage until it's all out?

int capped = std::min(mMaxValue, std::max(mValue, mMinValue));
if (capped != mValue)
{
mValue = capped;
setCaption(MyGUI::utility::toString(mValue));
}
}
catch (boost::bad_lexical_cast&)
catch (...)

This comment has been minimized.

@ghost

ghost Jun 10, 2017

Can we avoid using catch-all?

@ghost

ghost Jun 10, 2017

Can we avoid using catch-all?

This comment has been minimized.

@psi29a

psi29a Jun 10, 2017

Member

What do you suggest as a replacement?

@psi29a

psi29a Jun 10, 2017

Member

What do you suggest as a replacement?

This comment has been minimized.

@ghost

ghost Jun 10, 2017

Both of these, I guess?

If no conversion could be performed, an invalid_argument exception is thrown.

If the value read is out of the range of representable values by an int, an out_of_range exception is thrown.
@ghost

ghost Jun 10, 2017

Both of these, I guess?

If no conversion could be performed, an invalid_argument exception is thrown.

If the value read is out of the range of representable values by an int, an out_of_range exception is thrown.
@@ -220,7 +214,8 @@ namespace SceneUtil
// possible optimization: return a StateSet containing all requested lights plus some extra lights (if a suitable one exists)
size_t hash = 0;
for (unsigned int i=0; i<lightList.size();++i)
boost::hash_combine(hash, lightList[i]->mLightSource->getId());
hash = hash ^ (lightList[i]->mLightSource->getId() << 1); // or use boost::hash_combine

This comment has been minimized.

@ghost

ghost Jun 10, 2017

Is this really as good as hash_combine? Have you tested for a collision?

@ghost

ghost Jun 10, 2017

Is this really as good as hash_combine? Have you tested for a collision?

This comment has been minimized.

@psi29a

psi29a Jun 10, 2017

Member

It was faster but doesn't attempt to prevent the low-end and high-end bits from colliding.

We can just do what boost does, use the golden ratio to prevent (slow down) this collision.

template <class T>
inline void hash_combine(std::size_t& seed, const T& v)
{
    std::hash<T> hasher;
    seed ^= hasher(v) + 0x9e3779b9 + (seed<<6) + (seed>>2);
}

Do we want good enough or do we want 'the best' but slower.

@psi29a

psi29a Jun 10, 2017

Member

It was faster but doesn't attempt to prevent the low-end and high-end bits from colliding.

We can just do what boost does, use the golden ratio to prevent (slow down) this collision.

template <class T>
inline void hash_combine(std::size_t& seed, const T& v)
{
    std::hash<T> hasher;
    seed ^= hasher(v) + 0x9e3779b9 + (seed<<6) + (seed>>2);
}

Do we want good enough or do we want 'the best' but slower.

This comment has been minimized.

@ghost

ghost Jun 10, 2017

We want whatever was working before and has not caused problems.

@ghost

ghost Jun 10, 2017

We want whatever was working before and has not caused problems.

@@ -22,15 +24,15 @@ namespace Fallback
if(fallback.empty())
return 0;
else
return boost::lexical_cast<float>(fallback);
return std::stof(fallback);

This comment has been minimized.

@ghost

ghost Jun 10, 2017

lexical_cast and stof are not the same: g++ test.cpp -o test -std=c++11

#include <iostream>
#include <string>

#include <boost/lexical_cast.hpp>

int main()
{
	setlocale(LC_NUMERIC, "de_DE.UTF-8");

	float a = std::stof("1.5");
	float b = std::stof("1,5");

	std::cout << " a" << a << " b " << b << std::endl;

	try
	{
        	a = boost::lexical_cast<float>("1.5");
	}
	catch(...)
	{
		a = 0;
	}
	try
	{
        	b = boost::lexical_cast<float>("1,5");
	}
	catch(...)
	{
		b = 0;
	}

        std::cout << " a" << a << " b " << b << std::endl;

	return 0;
}

output:

 a1 b 1.5
 a1.5 b 0
  1. error handling: boost throws an exception, stof silently ignores parsing errors
  2. locale: boost::lexical_cast does not use current locale, stof does use it. We don't want to use it. (The default is 'C' usually, but AFAIK QT changes it).
@ghost

ghost Jun 10, 2017

lexical_cast and stof are not the same: g++ test.cpp -o test -std=c++11

#include <iostream>
#include <string>

#include <boost/lexical_cast.hpp>

int main()
{
	setlocale(LC_NUMERIC, "de_DE.UTF-8");

	float a = std::stof("1.5");
	float b = std::stof("1,5");

	std::cout << " a" << a << " b " << b << std::endl;

	try
	{
        	a = boost::lexical_cast<float>("1.5");
	}
	catch(...)
	{
		a = 0;
	}
	try
	{
        	b = boost::lexical_cast<float>("1,5");
	}
	catch(...)
	{
		b = 0;
	}

        std::cout << " a" << a << " b " << b << std::endl;

	return 0;
}

output:

 a1 b 1.5
 a1.5 b 0
  1. error handling: boost throws an exception, stof silently ignores parsing errors
  2. locale: boost::lexical_cast does not use current locale, stof does use it. We don't want to use it. (The default is 'C' usually, but AFAIK QT changes it).

This comment has been minimized.

@psi29a

psi29a Jun 10, 2017

Member

I can revert this unless we can find something else that does work.

@psi29a

psi29a Jun 10, 2017

Member

I can revert this unless we can find something else that does work.

This comment has been minimized.

@psi29a

psi29a Jun 10, 2017

Member

Thank you for catching that, I would not have thought to test that. Outside the US that the , is used instead of the .

@psi29a

psi29a Jun 10, 2017

Member

Thank you for catching that, I would not have thought to test that. Outside the US that the , is used instead of the .

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jun 10, 2017

I don't think there's any reasonable replacement for boost::filesystem.

ghost commented Jun 10, 2017

I don't think there's any reasonable replacement for boost::filesystem.

@psi29a

This comment has been minimized.

Show comment
Hide comment
@psi29a

psi29a Jun 10, 2017

Member

I haven't touched boost::filesystem for that reason. :/

Member

psi29a commented Jun 10, 2017

I haven't touched boost::filesystem for that reason. :/

@psi29a

This comment has been minimized.

Show comment
Hide comment
@psi29a

psi29a Jun 10, 2017

Member

I'll get a PR ready this evening or tomorrow morning at the latest.

Member

psi29a commented Jun 10, 2017

I'll get a PR ready this evening or tomorrow morning at the latest.

@psi29a psi29a referenced this pull request Jun 11, 2017

Merged

Fix deboosting #1318

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment