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

[ML] Do not log incorrect model memory limit #3

Merged
merged 2 commits into from Feb 23, 2018

Conversation

Projects
None yet
4 participants
@dimitris-athanasiou
Copy link
Contributor

commented Feb 22, 2018

Previously, the model memory limit was logged twice:
once when the CResourceMonitor was constructed and once
when the limit was read from the config file. The first
time the log message contained the default limit (4GB).
The second time it contained the actual limit.

The first message is incorrect. In addition, 6.3 is
changed so that the limit is always set explicitly. Thus,
there is no reason to keep logging in the constructor of
the monitor.

This commit removes the logging from the constructor of
the resource monitor.

[ML] Do not log incorrect model memory limit
Previously, the model memory limit was logged twice:
once when the CResourceMonitor was constructed and once
when the limit was read from the config file. The first
time the log message contained the default limit (4GB).
The second time it contained the actual limit.

The first message is incorrect. In addition, 6.3 is
changed so that the limit is always set explicitly. Thus,
there is no reason to keep logging in the constructor of
the monitor.

This commit removes the logging from the constructor of
the resource monitor.
@@ -58,7 +57,6 @@ CResourceMonitor::CResourceMonitor(std::size_t limit) : m_AllowAllocations(true)
m_PruneWindowMinimum(std::numeric_limits<std::size_t>::max()),
m_NoLimit(false)
{
this->memoryLimit(std::max(std::size_t(1), limit));

This comment has been minimized.

Copy link
@droberts195

droberts195 Feb 23, 2018

Contributor

If this line is being removed then we should remove the whole constructor. There's no point having a constructor that takes a limit argument but does nothing with it.

@@ -45,7 +45,6 @@ CResourceMonitor::CResourceMonitor(void) : m_AllowAllocations(true),
m_PruneWindowMinimum(std::numeric_limits<std::size_t>::max()),
m_NoLimit(false)
{
this->memoryLimit((sizeof(size_t) < 8) ? 1024: 4096);

This comment has been minimized.

Copy link
@droberts195

droberts195 Feb 23, 2018

Contributor

I'm surprised this doesn't mess up any unit tests, as it means a lot of unit tests will now be running with a resource monitor that hasn't inconsistently set member variables.

I would say this constructor should now initialise "no limit" case, i.e. set the following values in the initialiser list above:

m_ByteLimitHigh(std::numeric_limits<std::size_t>::max() / 2 + 1),
m_ByteLimitLow(m_ByteLimitHigh - 1024),
m_PruneThreshold(m_ByteLimitHigh / 5 * 3),
m_NoLimit(true)

This comment has been minimized.

Copy link
@dimitris-athanasiou

dimitris-athanasiou Feb 23, 2018

Author Contributor

I pushed a fix where I addressed this by doing something a bit different. Check it out and let me know if it's ok.

@droberts195
Copy link
Contributor

left a comment

LGTM

@dimitris-athanasiou dimitris-athanasiou merged commit 52533e4 into elastic:master Feb 23, 2018

dimitris-athanasiou added a commit that referenced this pull request Feb 23, 2018

[ML] Do not log incorrect model memory limit (#3)
Previously, the model memory limit was logged twice:
once when the CResourceMonitor was constructed and once
when the limit was read from the config file. The first
time the log message contained the default limit (4GB).
The second time it contained the actual limit.

The first message is incorrect. In addition, 6.3 is
changed so that the limit is always set explicitly. Thus,
there is no reason to keep logging in the constructor of
the monitor.

This commit removes the logging from the constructor of
the resource monitor.

@dimitris-athanasiou dimitris-athanasiou deleted the dimitris-athanasiou:do-not-log-incorrect-model-memory-limit branch Feb 23, 2018

@sophiec20 sophiec20 added the :ml label Apr 4, 2018

droberts195 pushed a commit that referenced this pull request Apr 23, 2018

[ML] Do not log incorrect model memory limit (#3)
Previously, the model memory limit was logged twice:
once when the CResourceMonitor was constructed and once
when the limit was read from the config file. The first
time the log message contained the default limit (4GB).
The second time it contained the actual limit.

The first message is incorrect. In addition, 6.3 is
changed so that the limit is always set explicitly. Thus,
there is no reason to keep logging in the constructor of
the monitor.

This commit removes the logging from the constructor of
the resource monitor.

droberts195 pushed a commit that referenced this pull request Apr 23, 2018

[ML] Do not log incorrect model memory limit (#3)
Previously, the model memory limit was logged twice:
once when the CResourceMonitor was constructed and once
when the limit was read from the config file. The first
time the log message contained the default limit (4GB).
The second time it contained the actual limit.

The first message is incorrect. In addition, 6.3 is
changed so that the limit is always set explicitly. Thus,
there is no reason to keep logging in the constructor of
the monitor.

This commit removes the logging from the constructor of
the resource monitor.

@lcawl lcawl added the >bug label Jun 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.