Ubuntu Application Launcher

CMakeLists.txt (+11/-3)
debian/control (+5/-5)
debian/libubuntu-app-launch3.install (+1/-1)
debian/libubuntu-app-launch3.shlibs (+1/-1)
helpers-shared.c (+1/-1)
helpers.c (+17/-8)
libubuntu-app-launch/CMakeLists.txt (+5/-1)
libubuntu-app-launch/app-info.h (+7/-1)
libubuntu-app-launch/application-impl-base.cpp (+463/-111)
libubuntu-app-launch/application-impl-base.h (+52/-5)
libubuntu-app-launch/application-impl-click.cpp (+30/-0)
libubuntu-app-launch/application-impl-click.h (+4/-0)
libubuntu-app-launch/application-impl-legacy.cpp (+37/-1)
libubuntu-app-launch/application-impl-legacy.h (+7/-0)
libubuntu-app-launch/application-impl-libertine.cpp (+28/-0)
libubuntu-app-launch/application-impl-libertine.h (+5/-0)
libubuntu-app-launch/application.cpp (+39/-0)
libubuntu-app-launch/application.h (+13/-0)
libubuntu-app-launch/libubuntu-app-launch.map (+1/-0)
libubuntu-app-launch/oom.h (+54/-0)
libubuntu-app-launch/registry-impl.cpp (+318/-0)
libubuntu-app-launch/registry-impl.h (+21/-2)
libubuntu-app-launch/registry.cpp (+61/-21)
libubuntu-app-launch/second-exec-core.c (+3/-10)
libubuntu-app-launch/second-exec-core.h (+2/-2)
libubuntu-app-launch/ubuntu-app-launch.cpp (+129/-462)
tests/CMakeLists.txt (+1/-0)
tests/libual-cpp-test.cc (+357/-74)
tests/libual-test.cc (+64/-20)

To merge this branch:

bzr merge lp://staging/~ted/ubuntu-app-launch/lp1579799-oom-adjust

Wishlist

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
dobey (community)		2016-08-08	Abstain on 2016-08-17
unity-api-1-bot	continuous-integration	2016-08-08	Needs Fixing on 2016-08-17
Charles Kerr (community)		2016-08-08	Approve on 2016-08-17
PS Jenkins bot	continuous-integration	2016-08-08	Pending
Review via email: mp+302321@code.staging.launchpad.net

This proposal supersedes a proposal from 2016-05-19.

Commit message

Add API to allow adjusting of the OOM score for an application instance

Description of the change

This branch is a bit of refactoring in that it moves the PID setting code from the C side to the C++ to add the new API. A lot of converted code that has been made more generic to accommodate the new use-cases. Then the C interface has been adjusted to call into the C++ code that implements the functionality.

The new code implements a way to set the OOM value for an instance. It uses a psuedo-enum style so that we can try to keep the states contained while still having a flexible system for experimentation.

A nice improvement in this code as that we're no longer using the ZG global log file so we don't have a memory leak from it if the registry is deallocated. Also the ZG event and DBus signalling now happen outside of the calling thread, which should improve the responsiveness of Unity8 (probably only slightly).

This branch does need a patch to ZG, specifically bug 1584849, which we will land in the same silo.

Revision history for this message

Gerry Boland (gerboland) wrote on 2016-05-23: Posted in a previous version of this proposal

API looks good!

Revision history for this message

Ted Gould (ted) wrote on 2016-05-24: Posted in a previous version of this proposal

Now have everything in a silo: https://requests.ci-train.ubuntu.com/#/ticket/1453

Revision history for this message

Charles Kerr (charlesk) wrote on 2016-05-24: Posted in a previous version of this proposal

comments inline

Revision history for this message

Ted Gould (ted) wrote on 2016-05-24: Posted in a previous version of this proposal

Download full text (6.3 KiB)

On Tue, 2016-05-24 at 19:36 +0000, Charles Kerr wrote:
> > # Deprecated needed for g_atexit() in libual
> > -set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Werror -Wno-
> > error=unused-function -Wno-error=deprecated-declarations
> > -std=gnu99")
> > -set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Werror -std=c++11
> > -pthread")
> > +set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Werror -Wno-
> > error=unused-function -Wno-error=deprecated-declarations -std=gnu99
> > -g")
> > +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Werror -std=c++11
> > -pthread -Wno-error=deprecated-declarations -g")
> Better way to do this is add_compile_options()
Okay, learned a bit. r254
> > /* Query lifecycle */
> > bool isRunning() override
> > {
> > - return ubuntu_app_launch_get_primary_pid(_appId.c_str()) != 0;
> > + return ubuntu_app_launch_get_primary_pid(std::string(appId_).c_str()) != 0;
> >

>
> "return primaryPid() != 0;" ?
>

Good idea! r253
> > - vector.push_back(static_cast<pid_t>(GPOINTER_TO_INT(list->data)));
> > + vector.push_back(static_cast<pid_t>(GPOINTER_TO_INT(pntr->data)));
> >

>
> Sneaky bugfix ;)
No comment :-)
> > /* Manage lifecycle */
> > void pause() override
> > {
> > - ubuntu_app_launch_pause_application(_appId.c_str());
> > + registry_->impl->zgSendEvent(appId_, ZEITGEIST_ZG_LEAVE_EVENT);
> > +
> > + auto oomstr = std::to_string(static_cast<std::int32_t>(oom::paused()));
> > + auto pids = forAllPids([this, oomstr](pid_t pid) {
> >

>
> better to capture oomstr by reference to avoid a temp copy
>

I think that the compiler should move it in that case because there's
no other usage, but to ensure I changed it (not gonna ready the asm to
prove it either way :-) ) r255
> > + auto serror = std::shared_ptr<GError>(error, g_error_free);
> > + throw std::runtime_error("Unable to access OOM value for '" + std::string(appId_) + "' primary PID '" +
> > + std::to_string(pid) + "' becuase: " + serror->message);
> >

>
> because
r256
> > + }
> > +
> > + auto score = static_cast<oom::Score>(std::atoi(content));
> >

>
> Bounds checking for [-1000..1000]?
Kinda felt like the kernel was doing that for us... seems we should be
able to depend on it?
> > - std::string _appId;
> > + AppID appId_;
> >

>
> const
>

r257
> > + {
> > + std::map<pid_t, bool> seenPids;
> >

>
>
> This would be better as a std::set. For example:
>
> std::set seenPids;
>
> bool added_any;
> do
> {
> added_any = false;
> auto pidlist = pids();
> for (auto& pid : pidlist)
> {
> if (seenPids.insert(pid).second)
> {
> eachPid(pid);
> added_any = true;
> }
> }
> }
> while (added_any);
>
> return std::vector(seenPids.begin(), seenPids.end());
Cool, good improvements. I kept the while instead of the do/while
because I think they confuse people as they're used so infrequently.
r258
> > + if (G_UNLIKELY(procpath.empty()))
> > + {
> > + /* Set by the test suite, probably not anyone else */
> > + ...

On Tue, 2016-05-24 at 19:36 +0000, Charles Kerr wrote:
> >  # Deprecated needed for g_atexit() in libual
> > -set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Werror -Wno-
> > error=unused-function -Wno-error=deprecated-declarations
> > -std=gnu99")
> > -set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Werror -std=c++11
> > -pthread")
> > +set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -Werror -Wno-
> > error=unused-function -Wno-error=deprecated-declarations -std=gnu99
> > -g")
> > +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Werror -std=c++11
> > -pthread -Wno-error=deprecated-declarations -g")
> Better way to do this is add_compile_options()
Okay, learned a bit. r254
> >      /* Query lifecycle */
> >      bool isRunning() override
> >      {
> > -        return ubuntu_app_launch_get_primary_pid(_appId.c_str()) != 0;
> > +        return ubuntu_app_launch_get_primary_pid(std::string(appId_).c_str()) != 0;
> >

> 
> "return primaryPid() != 0;" ?
>

Good idea! r253
> > -            vector.push_back(static_cast<pid_t>(GPOINTER_TO_INT(list->data)));
> > +            vector.push_back(static_cast<pid_t>(GPOINTER_TO_INT(pntr->data)));
> >

> 
> Sneaky bugfix ;)
No comment :-)
> >      /* Manage lifecycle */
> >      void pause() override
> >      {
> > -        ubuntu_app_launch_pause_application(_appId.c_str());
> > +        registry_->impl->zgSendEvent(appId_, ZEITGEIST_ZG_LEAVE_EVENT);
> > +
> > +        auto oomstr = std::to_string(static_cast<std::int32_t>(oom::paused()));
> > +        auto pids = forAllPids([this, oomstr](pid_t pid) {
> >

> 
> better to capture oomstr by reference to avoid a temp copy
>

I think that the compiler should move it in that case because there's
no other usage, but to ensure I changed it (not gonna ready the asm to
prove it either way :-) ) r255
> > +            auto serror = std::shared_ptr<GError>(error, g_error_free);
> > +            throw std::runtime_error("Unable to access OOM value for '" + std::string(appId_) + "' primary PID '" +
> > +                                     std::to_string(pid) + "' becuase: " + serror->message);
> >

> 
> because
r256
> > +        }
> > +
> > +        auto score = static_cast<oom::Score>(std::atoi(content));
> >

> 
> Bounds checking for [-1000..1000]?
Kinda felt like the kernel was doing that for us... seems we should be
able to depend on it?
> > -    std::string _appId;
> > +    AppID appId_;
> >

> 
> const
>

r257
> > +    {
> > +        std::map<pid_t, bool> seenPids;
> >

> 
> 
> This would be better as a std::set. For example:
> 
> std::set seenPids;
> 
> bool added_any;
> do
> {
>     added_any = false;
>     auto pidlist = pids();
>     for (auto& pid : pidlist)
>     {
>         if (seenPids.insert(pid).second)
>         {
>             eachPid(pid);
>             added_any = true;
>         }
>     }
> }
> while (added_any);
> 
> return std::vector(seenPids.begin(), seenPids.end());
Cool, good improvements. I kept the while instead of the do/while
because I think they confuse people as they're used so infrequently.
 r258
> > +        if (G_UNLIKELY(procpath.empty()))
> > +        {
> > +            /* Set by the test suite, probably not anyone else */
> > +            procpath = g_getenv("UBUNTU_APP_LAUNCH_OOM_PROC_PATH");
> >

> 
> This will crash if g_getenv() returns nullptr.\
>

Uhg, yes. r259
> > +                    g_warning("Unable to set OOM value for '%d' to '%s': %s", pid, oomvalue.c_str(),
> >

> 
> (pedantic): int(pid)
>

r260
> > 
> > +                              std::strerror(openerr));
> > +                    return;
> > +            }
> > +        }
> > +
> > +        size_t writesize = fwrite(oomvalue.c_str(), 1, oomvalue.size(), adj);
> > +        int writeerr = errno;
> > +        fclose(adj);
> > +
> > +        if (writesize == oomvalue.size())
> > +            return;
> > +
> > +        if (writeerr != 0)
> > +            g_warning("Unable to set OOM value for '%d' to '%s': %s", pid, oomvalue.c_str(), strerror(writeerr));
> > +        else
> > +            /* No error, but yet, wrong size. Not sure, what could cause this. */
> > +            g_debug("Unable to set OOM value for '%d' to '%s': Wrote %d bytes", pid, oomvalue.c_str(), (int)writesize);
> > +
> > +        return;
> >

> 
> The new code is inconsistent in its use of this return-at-end-of-function idiom
>

If it passes make format it is perfect.
r261
> > +        std::string pidstr = std::to_string(pid);
> > +        std::array<const char*, 4> args = {OOM_HELPER, pidstr.c_str(), oomvalue.c_str(), nullptr};
> > +
> > +        g_spawn_async(nullptr,               /* working dir */
> > +                      (char**)(args.data()), /* args */
> >

> 
> static_cast(args.data())
>

You'd think, right? Seems to not work. I don't think a static cast can
remove a const. Couldn't find a C++ way to do this.
> > +    /** Send a signal that we've change the application. Do this on the
> > +        registry thread in an idle so that we don't block anyone. */
> > +    void pidListToDbus(std::vector<pid_t>& pids, const std::string& signal)
> >

> 
> should pids be const? I can't tell.
>

I think so. r262
> > +    {
> > +        auto registry = registry_;
> > +        auto lappid = appId_;
> > +
> > +        registry_->impl->thread.executeOnThread([registry, lappid, pids, signal] {
> > +            auto vpids = std::shared_ptr<GVariant>(
> > +                [pids]() {
> > +                    GVariant* pidarray = nullptr;
> > +
> > +                    if (pids.size() == 0)
> >

> 
> if (pids.empty())
>

r263
> > +                    {
> > +                        pidarray = g_variant_new_array(G_VARIANT_TYPE_UINT64, NULL, 0);
> >

> 
> I'm unsure about this -- wouldn't we want to create an empty array instead?
>

Yes, I think that is creating an empty array.
> > +const Score fromLabelAndValue(std::int32_t value, const std::string& label);
> >

> 
> This doesn't seem to be used anywhere?
No, it's for the Unity8 folks to use.
> > +        zeitgeist_log_insert_event(zgLog_.get(), /* log */
> >

> 
> Doesn't look like we do anything interesting in the async handler.
> 
> Would it be simpler to use zeitgeist_log_insert_event_no_reply() here?
>

While we haven't had issues recently I'd prefer to get the debug
message if there is an error so we can figure things out. We had some
issues a while ago, nothing recently, but I don't think it hurts
anything.

Revision history for this message

Charles Kerr (charlesk) wrote on 2016-05-25: Posted in a previous version of this proposal

Thanks for the changes, ted!

review: Approve

Revision history for this message

Charles Kerr (charlesk) on 2016-05-25: Posted in a previous version of this proposal

review: Approve

Revision history for this message

unity-api-1-bot (unity-api-1-bot) wrote on 2016-07-11: Posted in a previous version of this proposal

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/7/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Gerry Boland (gerboland) wrote on 2016-07-19: Posted in a previous version of this proposal

This good to land then?

Revision history for this message

unity-api-1-bot (unity-api-1-bot) wrote on 2016-08-04: Posted in a previous version of this proposal

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/20/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

dobey (dobey) wrote on 2016-08-05: Posted in a previous version of this proposal

Currently failing to build. Think we also need to examine this to see how much we can split it up to smaller branches.

review: Needs Fixing

Revision history for this message

Ted Gould (ted) wrote on 2016-08-08:

This is the branch that needs the ZG fix to have the tests pass. Also, it has already been reviewed so I see no reason to split it up further.

Added the ABI tests which showed it is an ABI break, so added that in here.

Revision history for this message

dobey (dobey) wrote on 2016-08-08:

Can we split any of this out into smaller branches at all?

review: Needs Information

Revision history for this message

Ted Gould (ted) wrote on 2016-08-08:

On Mon, 2016-08-08 at 17:40 +0000, Rodney Dawes wrote:
> Can we split any of this out into smaller branches at all?
I think that effort would make sense to make it easier to review, but
since it has already been reviewed, I think at this point that effort
isn't warranted.

Revision history for this message

dobey (dobey) wrote on 2016-08-08:

> On Mon, 2016-08-08 at 17:40 +0000, Rodney Dawes wrote:
> > Can we split any of this out into smaller branches at all?
> I think that effort would make sense to make it easier to review, but
> since it has already been reviewed, I think at this point that effort
> isn't warranted.

It's not just about making it easier to review.

Revision history for this message

unity-api-1-bot (unity-api-1-bot) wrote on 2016-08-08:

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-ubuntu-app-launch-ci/44/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Charles Kerr (charlesk) on 2016-08-10:

review: Approve

Revision history for this message

dobey (dobey) wrote on 2016-08-10:

This needs split up into smaller changes. The ZG related fixes which are unrelated to OOM don't belong in this branch. The branch does not follow the style guidelines https://unity.ubuntu.com/wp-content/uploads/2012/03/cppguide.html#Formatting

review: Needs Fixing

Revision history for this message

Ted Gould (ted) wrote on 2016-08-10:

Please stop changing the status of this merge request. As stated earlier, I am not going to break it up into smaller branches. Sorry if that offends you.

Revision history for this message

dobey (dobey) wrote on 2016-08-10:

Well I'm sorry someone reviewed your giant mega branch 6 months ago and was a bit more lax about it, and then it sat around forever and you're using that as an excuse that it's already been reviewed, but I reviewed it and found problems and is not appropriate for landing into the tree in its current state. Sorry if that offends you.

lp://staging/~ted/ubuntu-app-launch/lp1579799-oom-adjust updated on 2016-08-16